Skip to content

Quality of life on stable channel #1084

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

Merged
merged 11 commits into from
Jul 20, 2022
Merged

Conversation

btielen
Copy link
Contributor

@btielen btielen commented Jul 18, 2022

While working on an integration for Axum, I experienced some inconveniences:

  1. In the root directory cargo test fails on rust-stable because
    • juniper_codegen_tests fails on stable
    • juniper_book_tests fails sometimes, I think because the life cycle of the build script in book/tests/build.rs is unpredictable.
  2. cargo fmt fails on rust stable, while CONTRIBUTING.md states that formatting has to be done on stable (like cargo clippy)
  3. cargo test -p juniper_integration_tests takes long to build and even crashed my computer a few times for some reason. See also this todo

Changes

This PR fixes these inconveniences by:

  • Only running the juniper_codegen_tests on nightly
  • Removing juniper_book_tests and using mdbook test instead
  • Moving back to formatting on stable
  • Splitting and running juniper_integration_tests in smaller units

Conclusion

  • Running cargo test from the root directory should now pass all tests on stable, beta and nightly
  • Building and testing time for juniper_integration_tests reduced from 3m56 to 2m04 on my computer
  • Allows testing of juniper_integration_tests on windows in the CI-pipeline
  • cargo clippy and cargo fmt should now be both run on stable

btielen added 5 commits July 18, 2022 13:12
- Run rustfmt on stable
- Remove package juniper_book_tests to test
- Add book_test job
- Run juniper_integration_tests also on windows
@btielen
Copy link
Contributor Author

btielen commented Jul 18, 2022

So mdbook test seems to fail on Windows, I was just able to confirm this on a local machine. We get this error:

LINK : fatal error LNK1181: cannot open input file 'windows.lib'

By removing (the unnecessary) extern crate tokio from line 85 book/src/advanced/subscriptions.md I was able to cut down one error. The other error on line 98 of book/src/types/objects/using_contexts.md also uses tokio. Maybe the feature sync for tokio was not activated?

Futher investigastion needed. I am not sure if this a bug in mdbook, rustdoc, the compiler or our code.

Edit: Same error using rustdoc book/src/types/objects/using_contexts.md --test --edition 2021 -L target/debug/deps, which indicates a bug in rustdoc or the compiler.

@tyranron tyranron added enhancement Improvement of existing features or bugfix k::toolchain Related to project toolchain labels Jul 19, 2022
@tyranron tyranron added this to the 0.16.0 milestone Jul 19, 2022
Copy link
Member

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@btielen thanks! Overall seems to be a nice life improvement, especially with book tests!

As for formatting on stable, I'd rather keep formatting on nightly. Stable formatting lacks some good features, we're intending to use. Just use make fmt for formatting. CONTRIBUTING.md just hasn't been updated for a while 😅

@btielen
Copy link
Contributor Author

btielen commented Jul 20, 2022

So it looks like the mdbook test failing on windows is caused by a bug in rustdoc. I issued a a bug report at the rust repo. I was wondering how a crate like Tokio is testing their documentation book. It turns out they are also using rustdoc, but their tests also completely fail on Windows. The doc tests are also not being run on windows on the CI-pipeline. For now I ignored the test that was causing the problems.

I made the requested changes.

@ilslv The juniper_codegen_tests now fail, although I didn't make any changes to them. Is it caused by a new version of rust nightly? Would it be possible to run the tests only on stable for more stability?

@ilslv
Copy link
Member

ilslv commented Jul 20, 2022

@btielen

Is it caused by a new version of rust nightly?

Almost certainly yes.

Would it be possible to run the tests only on stable for more stability?

I'm not sure about that. We use nightly, because there we emit unstable proc_macro diagnostics using proc_macro_error crate. You can run tests with env variable TRYBUILD=overwrite and it should automatically overwrite failing tests.

Copy link
Member

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@btielen thanks for the update! I'm going to merge this. I'll update the codegen tests issue.

@tyranron tyranron enabled auto-merge (squash) July 20, 2022 09:52
@tyranron tyranron merged commit bea9439 into graphql-rust:master Jul 20, 2022
@btielen btielen deleted the quality_of_life branch July 20, 2022 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix k::toolchain Related to project toolchain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants