Convert tests to pairs of Rust projects with their own manifests

[obi1kenobi]

Currently, test data is generated from a single project which has a (complicated) way to trigger semver violations by enabling or disabling its features.

In addition to being complicated and non-obvious, this also prevents us from semver-checking manifest information (#48) since there's currently only a single manifest.

It would be an improvement to switch to a test structure where each query is associated with a pair of Rust projects, complete with Cargo.toml files and their own source files:

/semver_tests/<query_name>/old/
/semver_tests/<query_name>/new/

Each of these two projects should have a crate name matching the query name, and the same version (e.g. 1.0.0).

To form a test case, one could create the "old" project, then cp -R old new and delete / alter code as necessary to cause the desired changes for the test. In this way, the new project is pretending to be the "next version" of the old project.

Running the tests would involve generating all the rustdoc JSON files as normal, and then asserting that cargo-semver-checks finds only the expected semver issues, with no false positives, in each pair of projects.

A crate like insta might be helpful here as well, since this is essentially a collection of snapshot tests.

[tonowak]

I'm thinking a bit about how to redesign the existing testing environment structure.

Currently the lints' queries are in src/queries, the .rs test files in semver_tests/src/test_cases and the output of the lints are in src/test_data.

I find it unintuitive (especially I thought so at the beginning, when I've tried to add my first lint).

The projects implements a layer of abstraction, thanks to which a lint is just a .ron file to query Trustfall. It doesn't have to do much with the code in src/, so it would make sense to move it out to a directory in the top-level directory (especially that someone might want to check how a given lint is written; in this case one wouldn't want to look at the other files in src/).

The semver_tests directory has a confusing name. It doesn't have any tests in them (in particular, I think one could be confused that there are no lint queries in this directory), but only some helper Rust crates on which the queries would be ran and tested.

One could wonder what the src/test_data has to do with the src folder. Well, not much, it's only related to src/queries and semver_tests/src/test_cases (and the relation with the second directory is not obvious from the structure of the directories). It makes sense for the outputs to be next to the queries, because it's a one-to-one relation with them, but it's missing the relation with semver_tests/src/test_cases.

The queries and the Rust crates are not a one-to-one relation -- one crate is sometimes used in multiple queries and probably after adding Manifest tests, some queries will use multiple crates. But nevertheless, they can be grouped together.

I propose the following directory structure:

• a toplevel lints directory (name chosen for people wanting to look into the queries, which they don't yet know that lints are Trustfall queries),
• directories inside lints that describe the category of lints (for example, parameter_count_changed for a query about a function's or method's parameters),
• inside a directory with any name, files in the format *.ron that previously were in src/queries/,
• in the same directory, files in the format *.test_output.ron that correspond to the appropriate query,
• in the same directory, some directories in the format test_crate_*_new and test_crate_*_old (or only test_crate_new and test_crate_old if there is only one needed crate) that would be equivalent to the proposed semver_tests/<query_name>/{old, new} in the issue's description.

The code for running the tests would be rewritten in a way to pick up the directories and queries from the described format. It would also be beneficial to have a README.md in lints/ describing how the lints work and how to add a new lint/test (and maybe some information about the necessity of running the script to regenerate rustdocs). This format would greatly reduce the number of steps needed to add a new test (it would be enough to create appropriate files/directories).

I see one potential drawback - one could think that the queries would be ran only on the corresponding crates from the same directory, but it could be overcome by describing in README.md that the queries are ran across all crates (which makes more sense, because it has a higher change to detect a bug).

What is your opinion @obi1kenobi?

[obi1kenobi]

I agree with the renaming of queries -> lints. I also agree that eventually we'd probably want more specific directories within lints to group related checks together, though I'm on the fence as to whether right now is the time for that. There's a benefit to simplicity in the structure, and I'm hesitant to over-do things especially all up front.

I have a few concerns:

The projects implements a layer of abstraction, thanks to which a lint is just a .ron file to query Trustfall. It doesn't have to do much with the code in src/, so it would make sense to move it out to a directory in the top-level directory (especially that someone might want to check how a given lint is written; in this case one wouldn't want to look at the other files in src/).

I agree that lint files don't have any real dependency on the Rust code in src/, I think that the inverse relationship is definitely there: Rust code in src/ will likely include_str!(...) the lint files. In my mind, that makes the lint files source code as well, so I think it would be good to leave them in src/.

in the same directory, files in the format *.test_output.ron that correspond to the appropriate query,

What happens if a lint flags code in more than one test project? How would we represent that?

Also, consider the workflow of adding new test projects (e.g. after adding a new lint). If the test output data sits adjacent to the lint file itself, then if the new test project also triggers some of the existing lints (as well as the newly-added one), then that PR will need to modify the test output files in the existing lint directory. This makes it look like the PR is adding and modifying multiple lints at once, and doesn't make it clear that it's actually only one lint + some new test data (with new test outputs) that are added.

I see one potential drawback - one could think that the queries would be ran only on the corresponding crates from the same directory

I think this is a major issue. In practice, people rarely read the README, especially when something seems intuitive and "clicks" in their mind. If I saw a query file and test case data in the same directory (and away from other similar query files and test data), it would absolutely "click" and I would 100% assume that's the only crate that is tested with that query. I feel strongly about not having the test data right adjacent to the lints because of this.

---

A few other things to consider:

• It might be worth looking at how clippy's directory structure is organized ( https://github.com/rust-lang/rust-clippy ), since it's a much bigger and more mature project that has very similar needs. We don't have to make all the same design decisions, but it's good to see a structure that's withstood the test of time where dozens of engineers have needed to work productively at the same time.
• It may be a good idea to include, in addition to the old / new projects, a third project (working name: witness) that demonstrates why something is a breaking change. It would have a dependency on the test project, and should compile fine when the old project is used to satisfy the dependency but fail to compile with the new project in its place. This is obviously only relevant for semver-major lints (and may only be feasible for some of them) and not relevant for semver-minor (i.e. non-breaking) ones.

[tonowak]

though I'm on the fence as to whether right now is the time for that.

Well, to solve this issue I need to change a bit the testing environment and it's easier/faster to implement other structural changes at the same time. So it's a good idea to think about it now, because I'll have the full context and the will to make the changes, and I'm not sure whether there'll be a better time.

I agree that lint files don't have any real dependency on the Rust code in src/, I think that the inverse relationship is definitely there: Rust code in src/ will likely include_str!(...) the lint files. In my mind, that makes the lint files source code as well, so I think it would be good to leave them in src/.

But then, it is possible to move out a file to the lints/ that does include_str! on all lints. Then no changes will be done to src/, because it would just include this file. The code is written to run some tests, deliberately without knowing what are those specific tests. Of course, it is not possible to completely remove the relation between the code and the tests, but it is as thin as it can be.

I agree with the renaming of queries -> lints

What about renaming the semver_tests?

What happens if a lint flags code in more than one test project? How would we represent that?

Good point. But I would expect the lints to be disjoint in the projects they detect. Aren't they?

then that PR will need to modify the test output files in the existing lint directory. This makes it look like the PR is adding and modifying multiple lints at once, and doesn't make it clear that it's actually only one lint + some new test data (with new test outputs) that are added.

I haven't fully understood your point. The lints won't be touched (besides the one added), but the outputs will be. Isn't that the same as with the current setup?

I would 100% assume that's the only crate that is tested with that query

Yeah, good point. It probably isn't a good idea after all.

a third project (working name: witness)

Makes sense. It would basically form a proof that this lint is needed. I could try implementing that. Should I?

[tonowak]

I guess the best time to test the witnesses would be in GitHub Actions? It doesn't seem important enough to test it during cargo test. Or maybe we should not even bother with testing the witnesses?

[obi1kenobi]

Well, to solve this issue I need to change a bit the testing environment and it's easier/faster to implement other structural changes at the same time. So it's a good idea to think about it now, because I'll have the full context and the will to make the changes, and I'm not sure whether there'll be a better time.

My gut feeling is that the directory with the lints should only contain lint files and nothing else (no test data etc.). Those lints should start off all being side-by-side in the same directory until we have a reason to add more nesting. This is also the direction clippy seems to have chosen, so unless we have good reason to not do the same, we probably should follow suit.

---

I'm fine with renaming semver_tests. I don't have a better name off the top of my head but I'm sure one can be found.

---

Good point. But I would expect the lints to be disjoint in the projects they detect. Aren't they?

Almost all of the time, they are. But in rare cases it can be quite difficult to write the test case in such a way that it only triggers a single lint.

In general, we want adding lints to be as easy as possible for anyone, even new contributors. Say we made this assumption, and threw in an assert!() to enforce it. Then a new contributor wants to add a new lint, and gets a bit unlucky and ends up with one of the lints where it's hard to write a test case that only triggers the lint intended to be tested. They have a bad time contributing, then they tell a few people that contributing was hard, and soon the project has a reputation of being hard to contribute to. This would be extremely unfortunate, and "common knowledge" like this is effectively impossible to reverse.

---

I think we should definitely test the witnesses somewhere. I'm not sure how long testing the witnesses would take, and it's possible it would take a relatively long time similar to the rustdoc JSON generation. If so, then we should have a script to test them outside of cargo test, then run the script on GitHub Actions while also allowing users to run the script locally if needed.

[tonowak]

One more thing which I want to resolve -- what about moving the lints/ to the top level directory?

Besides that, I'm on the same page and I agree with your arguments :)

[obi1kenobi]

The lints feel to me like they are source code: they certainly affect the execution of the program, and are even explicitly included (via include_str!()) into source code files.

Personally I'd leave them at src/lints/ because it feels strange to include into source code information from outside src/.

But I don't have strong feelings about this and if you feel strongly that lints/ should be outside src/ then feel free to go with that.