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

[red-knot] mdtest doesn't spot a new Markdown test file has been added until you force a rebuild of the crate being tested #13732

Closed
AlexWaygood opened this issue Oct 13, 2024 · 5 comments · Fixed by #13747
Assignees
Labels
red-knot Multi-file analysis & type inference testing Related to testing Ruff itself

Comments

@AlexWaygood
Copy link
Member

I just did the following:

  • Wrote some new red-knot code in a new PR branch
  • Added a new Markdown test for the code that I was sure was going to fail
  • Ran cargo test -p red_knot_python_semantic, was mystified at the lack of failures
  • After a little bit, figured out that the test wasn't being run at all
  • As soon as I added a random newline to red_knot_python_semantic, it triggered a rebuild of the crate and the new Markdown test file was picked up by mdtest, as expected.

I think this has actually happened to me a few times over the last few days, but this is the first time I realised exactly what was going on...

@AlexWaygood AlexWaygood added bug Something isn't working testing Related to testing Ruff itself red-knot Multi-file analysis & type inference labels Oct 13, 2024
@AlexWaygood
Copy link
Member Author

AlexWaygood commented Oct 13, 2024

I suppose this is because rstest generates the tests (one test per Markdown file, at least from rstest's perspective) at build time rather than at runtime here:

#[rstest::rstest]
fn mdtest(#[files("resources/mdtest/**/*.md")] path: PathBuf) {
let crate_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR"))
.join("resources")
.join("mdtest")
.canonicalize()
.unwrap();
let title = path.strip_prefix(crate_dir).unwrap();
run(&path, title.as_os_str().to_str().unwrap());
}

And it doesn't know to rebuild the tests when a Markdown file is added, because a Markdown file isn't a Rust source file.

@AlexWaygood
Copy link
Member Author

The rstest docs say:

Finally, often you would to recompile tests sources when file the folders or the environment variables changed. In this case you should provide a build.rs script file that tell to the compiler what to look in order to recompile the tests. For instance follow a simple example:

pub fn main() {
    println!("cargo::rerun-if-changed=tests/resources");
    println!("cargo::rerun-if-env-changed=BASE_TEST_DIR");
}

Which seems okay... though it's a bit of a pain if we have to add a build.rs script to every crate that uses mdtest-based tests. But maybe we won't actually end up adding many outside of red_knot_python_semantic?

@AlexWaygood
Copy link
Member Author

(Cc. @carljm)

@MichaReiser
Copy link
Member

Yeah. We discussed this in the notion design document. The only other alternative is to discover the tests in the test function itself but that has the downside that all markdown tests run sequentially (and one failing test prevents other markdown tests from running).

We should add a build.rs because not seeing your test after adding a file is very confusing.

@MichaReiser MichaReiser removed the bug Something isn't working label Oct 14, 2024
@Lexxxzy
Copy link
Contributor

Lexxxzy commented Oct 14, 2024

We should add a build.rs because not seeing your test after adding a file is very confusing.

+1, experienced the same problem while porting whole infer tests set on #13719.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference testing Related to testing Ruff itself
Projects
None yet
3 participants