-
Notifications
You must be signed in to change notification settings - Fork 459
Add a test for multiple contexts with Dune package management #13217
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
Conversation
Signed-off-by: Sudha Parimala <sudharg247@gmail.com>
rgrinberg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this tests needs to be re-organized.
-
Running the solver is completely unrelated to using contexts. So if you're trying to exercise how contexts work, there should be no need to invoke the solver. These two features do not interact.
-
There are already tests that build multiple lock directories with multiple contexts. Your test should demonstrate something that is not already present in those tests.
Are you talking about |
|
@Sudha247 Could you split off the documentation changes from this PR. That should be easy enough to review. The test however should be changed like Rudi suggests to use a hand-written lock. This should be straight forward by just catting what the real lock generation gave and inlining it. |
If that test isn't testing contexts correctly, then fixing it is the way to go. |
Signed-off-by: Sudha Parimala <sudharg247@gmail.com>
|
I’m happy to move the changes to the existing What I wanted to show, something the existing test doesn’t currently cover, is:
After discussing with @Alizter earlier, we weren’t sure these things were working, so it seemed useful to add coverage for them in the test suite after we discovered they were indeed working fine. |
|
I don't mean to say that the tests in this PR are redundant, but that they need to be re-organized.
Great, this can be its own test.
Constraints are only useful to the solver, so I'm not sure what you mean here.
depopts and contexts also do not interact. depopts interact with the solver to produce lock directories. Lock directories interact with contexts. Testing this end to end is unnecessary and redundant. Our tests should be focused on testing a single isolated feature or a particular interaction between features. So that if we break something, the test suite informs of what exactly is broken. We don't need more end to end tests because we already have way too many of them. We need tests that actually increase our coverage by testing parts that are not tested properly. There are tools (like ppx_bisect) to help with this, but I recommend to just get familiar with the kind of tests we already have. |
|
Closing in favour of #13231. |
This adds a test to show usage of multiple contexts, in the context of Dune package management.
The documentation was not super clear on how to do this. I've made some tweaks which hopefully makes it clearer, but I also intend to write a how-to guide for this with ThreadSanitizer and Dune package management as an example.