Split each source code in test_crates/ into pair of crates #203

tonowak · 2022-12-02T23:23:21Z

Closes #144

Unfortunately this PR is a bit tedious to review, sorry.

scripts/regenerate_test_rustdocs.sh

src/query.rs

obi1kenobi

I wanted to get some comments to you before it's too late in your timezone, and only managed to go through about half of this PR so far.

I'd highly recommend reading your own PR in the PR review interface. There are a bunch of things that stand out there that you probably wouldn't notice in your editor, and I bet you'll catch a number of issues and areas in need of polish and improvement by doing that. This is a high-risk PR since it changes so many things at once, and having more eyes reviewing it will also make it less likely that we end up with bugs as a result.

src/query.rs

test_crates/enum_repr_int_changed/new/src/main.rs

test_crates/enum_struct_variant_field_missing/new/src/main.rs

test_crates/item_missing/new/src/main.rs

test_crates/item_missing/new/Cargo.toml

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

test_outputs/enum_missing.output.ron

test_outputs/inherent_method_missing.output.ron

test_outputs/struct_missing.output.ron

test_outputs/method_parameter_count_changed.output.ron

src/query.rs

test_crates/item_missing/new/Cargo.toml

test_crates/enum_struct_variant_field_missing/new/src/main.rs

test_outputs/enum_repr_int_removed.output.ron

src/query.rs

test_crates/README.md

obi1kenobi · 2022-12-07T06:27:45Z

Merged main into this branch to pull in the update to trustfall v0.1.1 which will resolve at least one of the issues this PR ran into.

test_outputs/method_parameter_count_changed.output.ron

tonowak · 2022-12-09T17:25:44Z

Edit: done in bd42fd9.

The CONTRIBUTING.md needs to be updated (after merging #216 )

test_crates/README.md

tonowak · 2022-12-09T17:34:14Z

The CONTRIBUTING.md needs to be updated (after merging #216 )

Maybe it's better to move test_crates/README.md to CONTRIBUTING.md @obi1kenobi ?

…put assert

test_outputs/struct_marked_non_exhaustive.output.ron

tonowak · 2022-12-10T14:32:43Z

It might be beneficial to create the witness crates in a separate PR.

tonowak · 2022-12-10T14:35:01Z

Edit: done in 03c8333

I believe that cargo clippy and cargo fmt needs to be ran on each crate separately.

src/query.rs

tonowak · 2022-12-10T14:45:42Z

test_crates/pub_use_handling/old/src/lib.rs

@@ -0,0 +1 @@
+pub struct CheckPubUseHandling;


Now that I'm trying to understand what it's doing, it should be in a inner mod?

@obi1kenobi

Yes, to mirror the old structure it should be in a pub mod inner module and have a pub use in lib.rs.

Really this test doesn't need to have old/new versions. It's more just testing that re-exports are detected correctly — the re-exports are why this test case (which you said you were unsure about) exists and used to include multiple import paths for this CheckPubUseHandling struct.

But there's no problem with having this in an old/new configuration as well. Just add the pub mod inner and pub use in both the old and the new crates, and we can test that way.

Is this what you had in mind, or should I also put all of the code into an inner crate, just like it was in the previous test structure? 0f9b7ec

test_crates/struct_missing/old/new/Cargo.toml

test_crates/struct_pub_field_missing/new/src/lib.rs

test_outputs/enum_repr_int_removed.output.ron

obi1kenobi · 2022-12-11T04:08:24Z

The CONTRIBUTING.md needs to be updated (after merging #216 )

Maybe it's better to move test_crates/README.md to CONTRIBUTING.md @obi1kenobi ?

I'd say move the contributing-specific information to CONTRIBUTING.md but also have a README.md because that's what GitHub shows when you open that directory, and because not all the info that belongs in that file is contributing-related.

What do you think?

obi1kenobi

Overall, it's great: it has already helped flag multiple false-positive issues, and the test crate code is much more readable on account of not having to mess with features and conditional compilation.

I flagged a few minor things that should be easy to fix, and we need to remember to open a few GitHub issues so we don't forget to resolve the lint issues we've flagged. If you get a chance to open them — great! If not, I'll open them when it's not 1am over here 😴

scripts/regenerate_test_rustdocs.sh

obi1kenobi · 2022-12-11T04:20:18Z

scripts/regenerate_test_rustdocs.sh

+# Run rustdoc on test_crates/*/{new,old}/
+for crate_pair in $(find "$TOPLEVEL/test_crates/" -maxdepth 1 -mindepth 1 -type d); do
+	# Removing path prefix, leaving only the directory name without forward slashes
+	crate_pair=${crate_pair#"$TOPLEVEL/test_crates/"}


Suggested change

crate_pair=${crate_pair#"$TOPLEVEL/test_crates/"}

crate_pair="${crate_pair#"$TOPLEVEL/test_crates/"}"

Should this be quoted?

Why shouldn't this be quoted? Maybe the crate_pair would have spaces in it, for whatever reasons. Though I hope that the quotation marks match with each other correctly in the old version.

I think it should be quoted -- my suggested edit just wraps the whole thing in one extra set of quotes. That's just something I do obsessively in bash scripts since I've debugged several too many scripts where missing quotes either broke something (less bad option) or enabled script injection (much worse option). I just was wondering if adding the extra quotes might break something that I didn't notice.

Oh sorry, I've misunderstood. I thought you wanted to delete the quotes. I'll change it in a separate PR.

obi1kenobi · 2022-12-11T04:32:31Z

test_crates/pub_use_handling/old/src/lib.rs

@@ -0,0 +1 @@
+pub struct CheckPubUseHandling;


Yes, to mirror the old structure it should be in a pub mod inner module and have a pub use in lib.rs.

Really this test doesn't need to have old/new versions. It's more just testing that re-exports are detected correctly — the re-exports are why this test case (which you said you were unsure about) exists and used to include multiple import paths for this CheckPubUseHandling struct.

But there's no problem with having this in an old/new configuration as well. Just add the pub mod inner and pub use in both the old and the new crates, and we can test that way.

src/query.rs

test_crates/README.md

test_outputs/struct_marked_non_exhaustive.output.ron

test_outputs/method_parameter_count_changed.output.ron

test_outputs/struct_missing.output.ron

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

tonowak · 2022-12-11T09:40:53Z

I'd say move the contributing-specific information to CONTRIBUTING.md but also have a README.md because that's what GitHub shows when you open that directory, and because not all the info that belongs in that file is contributing-related.

Done.

I flagged a few minor things that should be easy to fix, and we need to remember to open a few GitHub issues so we don't forget to resolve the lint issues we've flagged. If you get a chance to open them — great! If not, I'll open them when it's not 1am over here sleeping

I'll create the issues after merging this PR, to reference the exact lines of codes in the main branch.
I plan to create the following issues:

witness crates,
method_parameter_count_changed lint false-positive,
struct_missing lint false-posivite,
struct_marked_non_exhaustive lint false-positive.

Is there anything I've missed?

obi1kenobi · 2022-12-16T19:30:29Z

Another idea I had for automating false-positive detection in the future:

Some crates intentionally include items that are crafted as "bait" for false-positives, e.g. to act as regression tests for past false-positive issues. If we always place such "bait" in a dedicated file / module (e.g. false_positive.rs), then any time a rule reports an item from its own crate's false_positive module, we know that's a bug.

Other crates are allowed to report on false_positive modules' items, since those items may trigger other lints. For example, the false-positive where deleted methods get reported as changing their number of parameters: if that test case existed in false_positive.rs test crates for the lint for changing parameter count, it would have correctly been reported as a missing method by the lint that looks for those instead.

obi1kenobi · 2022-12-17T00:04:43Z

I'll create the issues after merging this PR, to reference the exact lines of codes in the main branch. I plan to create the following issues:

witness crates,

method_parameter_count_changed lint false-positive,

struct_missing lint false-posivite,

struct_marked_non_exhaustive lint false-positive.

I hit a burst of productivity today, so I debugged and fixed all three false-positives, and opened the witness crates issue 🚀

Split each source code in test_crates/ into pair of crates

feb95b1

tonowak requested a review from obi1kenobi December 2, 2022 23:23

tonowak mentioned this pull request Dec 3, 2022

Run lints on nonchanged crates #206

Merged

obi1kenobi reviewed Dec 3, 2022

View reviewed changes

Review changes

a2e7f9c

obi1kenobi reviewed Dec 4, 2022

View reviewed changes

tonowak and others added 5 commits December 4, 2022 21:03

Changed expect string style

e43fe7c

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

Changed expect string style

3bdaf43

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

Removed some small stuff I missed during splitting

bd42fd9

Merge branch 'main' into split_test_crates

5cc0964

Regenerated test outputs

1cbb9dd

tonowak commented Dec 5, 2022

View reviewed changes

Added in

89fd8e2

tonowak commented Dec 5, 2022

View reviewed changes

src/query.rs Outdated Show resolved Hide resolved

tonowak commented Dec 5, 2022

View reviewed changes

test_crates/README.md Show resolved Hide resolved

Merge branch 'main' into split_test_crates

98b28a9

tonowak added 2 commits December 7, 2022 16:14

Changed script to ignore README.md in test_crates/

ce34cab

Regenerated output after trustfall_core bugfix

2986eca

tonowak commented Dec 7, 2022

View reviewed changes

test_outputs/method_parameter_count_changed.output.ron Outdated Show resolved Hide resolved

tonowak commented Dec 9, 2022

View reviewed changes

test_crates/README.md Outdated Show resolved Hide resolved

tonowak added 4 commits December 9, 2022 19:01

Split item_missing into multiple crate pairs

d7497d9

Created a map of results instead of flattening them, changed test out…

4394090

…put assert

Forgot to fmt

e8e990d

Renamed all src/main.rs to src/lib.rs in test crate pairs

9795cd7

tonowak commented Dec 9, 2022

View reviewed changes

test_outputs/struct_marked_non_exhaustive.output.ron Outdated Show resolved Hide resolved

tonowak added 2 commits December 9, 2022 22:52

Seems clippy doesn't work locally for me, I'll need to inspect this

1dc2049

Changed key values in top-level map in outputs

8cfa0b8

tonowak commented Dec 10, 2022

View reviewed changes

tonowak added 5 commits December 10, 2022 16:12

Updated test_crates/README.md

ae210f2

Merge branch 'main' into split_test_crates

6912486

Updated CONTRIBUTING.md

9b67693

Self-review

1ea9a64

Ran clippy and fmt on crate pairs

03c8333

obi1kenobi reviewed Dec 11, 2022

View reviewed changes

tonowak and others added 6 commits December 11, 2022 09:11

Shortened iteration over map

ddc88ec

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

Changed wording in test_crates/README.md

2dca44f

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

Attempt at fixing pub_use_handling test

0f9b7ec

Moved a description from README.md to CONTRIBUTING.md`

c2f1b54

Changed tabs to spaces

1e55360

Changed some unwraps to expects

d2fb843

SmolSir mentioned this pull request Dec 12, 2022

New schema for attributes #171

Merged

tonowak mentioned this pull request Dec 13, 2022

Trailing commas and white characters in #[repr(...)] attributes #217

Open

obi1kenobi approved these changes Dec 13, 2022

View reviewed changes

obi1kenobi merged commit ed46162 into obi1kenobi:main Dec 13, 2022

This was referenced Dec 16, 2022

"Struct missing" lint shouldn't care about the kind of struct. #218

Merged

Do not report removed methods as changing their number of parameters. #220

Merged

This was referenced Dec 16, 2022

Document private items to fix non-exhaustive false-positives. #222

Merged

Add witness crates that demonstrate how each breaking change is breaking #223

Open

obi1kenobi mentioned this pull request Dec 22, 2022

Tracking issue: Improvements to cargo-semver-checks' own CI and test system #225

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split each source code in test_crates/ into pair of crates #203

Split each source code in test_crates/ into pair of crates #203

tonowak commented Dec 2, 2022 •

edited

Loading

obi1kenobi left a comment

obi1kenobi commented Dec 7, 2022

tonowak commented Dec 9, 2022 •

edited

Loading

tonowak commented Dec 9, 2022

tonowak commented Dec 10, 2022 •

edited

Loading

tonowak commented Dec 10, 2022 •

edited

Loading

tonowak Dec 10, 2022

tonowak Dec 10, 2022

obi1kenobi Dec 11, 2022

tonowak Dec 11, 2022

obi1kenobi commented Dec 11, 2022

obi1kenobi left a comment

obi1kenobi Dec 11, 2022

tonowak Dec 11, 2022

obi1kenobi Dec 13, 2022

tonowak Dec 14, 2022

obi1kenobi Dec 11, 2022

tonowak commented Dec 11, 2022

obi1kenobi commented Dec 16, 2022

obi1kenobi commented Dec 17, 2022

	crate_pair=${crate_pair#"$TOPLEVEL/test_crates/"}
	crate_pair="${crate_pair#"$TOPLEVEL/test_crates/"}"

Split each source code in test_crates/ into pair of crates #203

Split each source code in test_crates/ into pair of crates #203

Conversation

tonowak commented Dec 2, 2022 • edited Loading

obi1kenobi left a comment

Choose a reason for hiding this comment

obi1kenobi commented Dec 7, 2022

tonowak commented Dec 9, 2022 • edited Loading

tonowak commented Dec 9, 2022

tonowak commented Dec 10, 2022 • edited Loading

tonowak commented Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi commented Dec 11, 2022

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonowak commented Dec 11, 2022

obi1kenobi commented Dec 16, 2022

obi1kenobi commented Dec 17, 2022

tonowak commented Dec 2, 2022 •

edited

Loading

tonowak commented Dec 9, 2022 •

edited

Loading

tonowak commented Dec 10, 2022 •

edited

Loading

tonowak commented Dec 10, 2022 •

edited

Loading