Skip to content

Check that a test stanza is disabled before generating the rules#6134

Closed
voodoos wants to merge 3 commits intoocaml:mainfrom
voodoos:disabled-test-stanza
Closed

Check that a test stanza is disabled before generating the rules#6134
voodoos wants to merge 3 commits intoocaml:mainfrom
voodoos:disabled-test-stanza

Conversation

@voodoos
Copy link
Copy Markdown
Collaborator

@voodoos voodoos commented Sep 7, 2022

Unlike the executables stanza, Dune attempted to create some rules for a tests even if it was disabled. This adds a similar check as the one for executables to abort rule generation if the stanza is disabled.

This should fix #6132

@voodoos voodoos force-pushed the disabled-test-stanza branch from d9e2a44 to 94ea351 Compare September 7, 2022 16:37
@rgrinberg rgrinberg added this to the 3.5.0 milestone Sep 8, 2022
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

Please describe the test and add a CHANGES entry.

voodoos added a commit to voodoos/dune that referenced this pull request Sep 8, 2022
@voodoos voodoos force-pushed the disabled-test-stanza branch from 94ea351 to 5e456e1 Compare September 8, 2022 08:57
When a test is disabled dune shouldn't check the availability of its
dependencies.

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
This is what is done for executables: no rule is generated if the enabled_if field evaluates to false.

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos voodoos force-pushed the disabled-test-stanza branch from 5e456e1 to 3095ca7 Compare September 8, 2022 10:19
@voodoos
Copy link
Copy Markdown
Collaborator Author

voodoos commented Sep 8, 2022

Thanks for the review Rudi !
I added a comment and a changelog entry.

@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Sep 9, 2022

It's a dupe of #5529 isn't it? In which case the same review comment applies - we want to preserve the behavior and somehow version it or add a knob to (test) to say what (enabled_if) applies to.

@voodoos
Copy link
Copy Markdown
Collaborator Author

voodoos commented Sep 13, 2022

It is in fact a duplicate... with exactly the same changes, so there is no reason to keep this PR open.

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.

support recursive enable_if in tests

3 participants