Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix override_settings issue #204

Merged
merged 3 commits into from
Mar 10, 2023
Merged

Conversation

ewjoachim
Copy link

See #203 for context.

Also, this is enough to fix the issue in the reproduction repo.

@codingjoe
Copy link
Owner

I will close this until we have reached a conses in the issue, OK?

Copy link
Owner

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ewjoachim,

Thanks again for the detailed description. Would you mind adding the test you wrote here? You can add Django unit tests to pytest. They will still be picked up and executed.

Best!
Joe

django_select2/apps.py Outdated Show resolved Hide resolved
Co-authored-by: Johannes Maron <[email protected]>
@ewjoachim
Copy link
Author

You can add Django unit tests to pytest

I don't need to, I'm pretty sure it happens identically with Pytest. The only reason I didn't use pytest on the repro repo was to keep things as simple as possible :)

@ewjoachim
Copy link
Author

ewjoachim commented Feb 11, 2023

Ok, if you really want to have a test on this we're going to need black magic or a very specific setup, and in both cases it's going to be very brittle.

In the tests, as they currently exist, the conf module is loaded as a side effect of importing other modules, which happens during test collection, before tests actually start running.
The only ways I see to reproduce the issue would be:

  • To have our test run before other tests are collected. The only way to achieve this is to run this test in a dedicated pytest process. That's a very specific setup, it means we can't run all the test just by launching pytest, we'd need to launch both pytest on 1 subfolder and then pytest on everything
  • Or to force Python to ignore that conf has already been loaded, e.g. by using reload() or deleting stuff from sys.modules manually. I'm calling this black magic, and as you know, all magic comes with a price (and I'm not sure I'm ready to pay the price)
  • (or the test would run a python subprocess, but it's awful)
  • (or we would remove all top-level imports of select2 in your other tests, but that's going to be very ugly. We could do that with a fixture but... 🤢 )

Also, any setup (except possibly the subprocess) is probably going to be very brittle: if the setup doesn't work, it means the test will always pass. I'm not sure there's a way to write a test so that if you inadvertently destroy the setup in a following PR, you'll still be testing something relevant. So if someone removes the fix in the future, it's likely that no test would fail.

If you have another idea, I'm all ears.

To be super duper extra clear: We can't write a test if we can't manage to achieve the following setup:

  • Add a breakpoint() in conf.py at the module level
  • Add a breakpoint() in a new test
  • do some magic
  • when launching test, if the 2nd breakpoint()hits before the first, then the magic worked and we can write our test.

@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +1.12 🎉

Comparison is base (37e9f7f) 98.14% compared to head (bb15975) 99.27%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #204      +/-   ##
==========================================
+ Coverage   98.14%   99.27%   +1.12%     
==========================================
  Files           7        7              
  Lines         270      276       +6     
==========================================
+ Hits          265      274       +9     
+ Misses          5        2       -3     
Impacted Files Coverage Δ
django_select2/apps.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@codingjoe codingjoe merged commit f9a439d into codingjoe:main Mar 10, 2023
@ewjoachim ewjoachim deleted the fix-203-appconf branch March 10, 2023 22:15
@ewjoachim
Copy link
Author

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants