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

futures: Require all stable features in tests #2216

Merged
merged 1 commit into from
Sep 30, 2020

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Sep 22, 2020

In projects that have many feature flags and tests, it is hard to maintain the support of the feature flag combination in tests (due to complex cfgs, long test time, etc.), so it is probably better to fail tests if all stable features are not activated.

Related: #2101
Closes: #2098

cc @cramertj @Nemo157 @kentfredric

@taiki-e taiki-e force-pushed the test-cfg branch 2 times, most recently from 0658fc3 to b2cdebb Compare September 22, 2020 09:47
@kentfredric
Copy link
Contributor

A workaround for "the tests take a long time" might be to set up a "cron" driven CI test, so they don't demand feature combination with PRs and soforth, but a reminder to keep on top of it still happens.

@taiki-e
Copy link
Member Author

taiki-e commented Sep 22, 2020

Even if we don't do it in PR, someone has to do it to fix if cron fails...

@kentfredric
Copy link
Contributor

Yeah, just the downside really is without testing feature combos, you have no evidence features can be arbitrarily combined in a dependent.

So whether or not you have tests that break, you can still have code that breaks, no?

@taiki-e
Copy link
Member Author

taiki-e commented Sep 30, 2020

Our CI already does feature flag check, and, IIRC, all the current futures's stable feature flags are additive (not only APIs but also implementations). So this PR shouldn't have a negative impact on the actually published crate.

@taiki-e taiki-e merged commit 5b21883 into rust-lang:master Sep 30, 2020
@taiki-e taiki-e deleted the test-cfg branch September 30, 2020 09:58
@kentfredric
Copy link
Contributor

Our CI already does feature flag check, and, IIRC, all the current futures's stable feature flags are additive (not only APIs but also implementations). So this PR shouldn't have a negative impact on the actually published crate.

That looks adequate for ensuring everything compiles, sure, but doesn't give many assurances of expected behaviour.

Just restricting this this way makes it harder to test things downstream in a comprehensive way. ( which is a relatively important thing to do, when you're a linux vendor )

@Nemo157
Copy link
Member

Nemo157 commented Oct 1, 2020

Part of Rusts feature design is that features do not change behaviour they only expand the available API. As far as I can tell futures strictly follows this design principle, so that testing with all features active tests the exact same code paths that would be tested with some features inactive.

@kentfredric
Copy link
Contributor

Fair enough, and I hope you're right :). I've just tended to preferred to be able to prove things via evidence, as opposed to constructing a conjecture that it should be ok, for I find that, however reasonable we are when making such a conjecture, humans always possess the risk of missing something, which only an evidential test will expose. :)

taiki-e added a commit to taiki-e/futures-rs that referenced this pull request Feb 26, 2021
After rust-lang#2216, these redundant imports are unneeded.
This reverts almost all of rust-lang#2101.
taiki-e added a commit to taiki-e/futures-rs that referenced this pull request Feb 26, 2021
After rust-lang#2216, these redundant imports are unneeded.
This reverts almost all of rust-lang#2101.
taiki-e added a commit to taiki-e/futures-rs that referenced this pull request Feb 26, 2021
After rust-lang#2216, these redundant imports are unneeded.
This reverts almost all of rust-lang#2101.
This also includes some minor cleanup of imports.
taiki-e added a commit to taiki-e/futures-rs that referenced this pull request Feb 26, 2021
After rust-lang#2216, these redundant imports are unneeded.
This reverts almost all of rust-lang#2101.
This also includes some minor cleanup of imports in tests.
taiki-e added a commit to taiki-e/futures-rs that referenced this pull request Feb 26, 2021
After rust-lang#2216, these redundant imports are unneeded.
This reverts almost all of rust-lang#2101.
This also includes some minor cleanup of imports in tests.
taiki-e added a commit that referenced this pull request Feb 26, 2021
After #2216, these redundant imports are unneeded.
This reverts almost all of #2101.
This also includes some minor cleanup of imports in tests.
exrook pushed a commit to exrook/futures-rs that referenced this pull request Apr 5, 2021
After rust-lang#2216, these redundant imports are unneeded.
This reverts almost all of rust-lang#2101.
This also includes some minor cleanup of imports in tests.
taiki-e added a commit to taiki-e/futures-rs that referenced this pull request Apr 10, 2021
After rust-lang#2216, these redundant imports are unneeded.
This reverts almost all of rust-lang#2101.
This also includes some minor cleanup of imports in tests.
taiki-e added a commit that referenced this pull request Apr 10, 2021
After #2216, these redundant imports are unneeded.
This reverts almost all of #2101.
This also includes some minor cleanup of imports in tests.
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.

4 participants