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

Check that tests exist before trying to build them #233

Closed

Conversation

GuillaumeGomez
Copy link

I encountered this bug recently in my fork (which adds inline tests and sub-checks): the first time tests are run, they fail because they don't exist. The bug is actually present in your version as well, it's just not as bad as in my fork since the tests are supposed to already exist in your case.

So this fix simply moves the check of the tests existence before trying to build them.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I am not clear on what this is fixing. Could you share a way that I could reproduce the incorrect behavior?

@GuillaumeGomez
Copy link
Author

Add multiple tests with one that doesn't exist. You'll get the cargo error before you'll get the check_exists error, making the call to check_exists useless in run_all.

@dtolnay
Copy link
Owner

dtolnay commented Apr 13, 2023

I tried again and I am still not able to reproduce any test that would produce different behavior before and after this PR.

Please share a complete compilable test.rs, compiler version (stable vs nightly follow slightly different implementation because nightly can compile all tests in parallel), and before/after screenshots. I want to be able to run cargo test and observe what you are saying.

@GuillaumeGomez
Copy link
Author

I'll write how to reproduce then:

  1. Use https://crates.io/crates/trybuild2 (which doesn't include the equivalent fix of this PR that you can see here)
  2. Clone https://github.com/gtk-rs/gtk-rs-core locally
  3. Go to glib-macros subfolder
  4. Run cargo test clone_failures (which you can see here)

On the first run, the tests won't exist (because they need to be generated) and on the first run it'll work. It illustrates what this PR fixes: check_exists is called after the cargo run, making it either useless or too late.

@dtolnay
Copy link
Owner

dtolnay commented Apr 13, 2023

That repro doesn't even involve this crate.

I am going to assume this fix is not relevant to this crate.

@dtolnay dtolnay closed this Apr 13, 2023
@GuillaumeGomez GuillaumeGomez deleted the check-test-before-build branch April 13, 2023 17:59
@GuillaumeGomez
Copy link
Author

It is but whatever... Thanks in any case for writing this crate.

@GuillaumeGomez
Copy link
Author

For completeness:

  1. You run all tests https://github.com/dtolnay/trybuild/blob/master/src/run.rs#L346
  2. You check all tests exist (9 lines below) https://github.com/dtolnay/trybuild/blob/master/src/run.rs#L355

Repository owner locked and limited conversation to collaborators Apr 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants