Rewrite the `no-input-file.stderr` test in Rust and support diff #124257

JoverZhang · 2024-04-22T11:32:59Z

Rewrite the no-input-file.stderr test from #121876.
Use the similar lib to replace the diff command.

rustbot · 2024-04-22T11:33:07Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jieyouxu (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

src/tools/run-make-support/src/diff/mod.rs

JoverZhang · 2024-04-22T11:35:46Z

src/tools/run-make-support/src/diff/tests.rs

@@ -0,0 +1,38 @@
+#[cfg(test)]
+mod tests {


I think the tests here will not be executed in CI for now (I don't know how to configure it).

I think that is fine for now, but if you want to add support for running the unit tests in run-make-support, you'll need to do something like CompiletestTest:

rust/src/bootstrap/src/core/build_steps/test.rs

Line 731 in 7f2fc33

impl Step for CompiletestTest {

Modulo the unstable test feature part because run-make-support is stable-only and does not use any unstable (cargo or rustc) features.

I filed issue #124267 to track this.

src/tools/run-make-support/src/diff/tests.rs

rustbot · 2024-04-22T13:33:54Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

The run-make-support library was changed

cc @jieyouxu

Some changes occurred in run-make tests.

cc @jieyouxu

klensy · 2024-04-22T16:30:19Z

dissimilar already in tree, maybe use it?

jieyouxu

Thanks for the PR!

If we have dissimilar in tree already, we should use dissimilar as suggested to avoid introducing another similar (heh) dependency.

src/tools/run-make-support/src/diff/mod.rs

jieyouxu · 2024-04-22T16:14:47Z

src/tools/run-make-support/src/diff/tests.rs

@@ -0,0 +1,38 @@
+#[cfg(test)]
+mod tests {


I think that is fine for now, but if you want to add support for running the unit tests in run-make-support, you'll need to do something like CompiletestTest:

rust/src/bootstrap/src/core/build_steps/test.rs

Line 731 in 7f2fc33

impl Step for CompiletestTest {

Modulo the unstable test feature part because run-make-support is stable-only and does not use any unstable (cargo or rustc) features.

jieyouxu · 2024-04-22T16:19:13Z

src/tools/run-make-support/src/diff/tests.rs

@@ -0,0 +1,38 @@
+#[cfg(test)]
+mod tests {


I filed issue #124267 to track this.

src/tools/run-make-support/src/diff/tests.rs

src/tools/run-make-support/src/diff/mod.rs

tests/run-make/no-input-file/rmake.rs

JoverZhang · 2024-04-23T16:59:09Z

dissimilar already in tree, maybe use it?

@klensy @jieyouxu Thank you for your suggestion. I value your advice and took the time to explore how to use dissimilar to output the diff format. However, it seems this approach may not work. I found that dissimilar only allows comparison based on char tokens, whereas diff should ideally perform comparisons starting with lines as tokens.

For example, when we use dissimilar::diff("bar", "baz"), we get:

  ba
- r
+ z

But what we expect is:

- bar
+ baz

I also thought about encoding all lines first (into a different Unicode char) and then calling dissimilar::diff(). But I don't think that's very elegant. :(

If you have any better suggestions, please let me know at your earliest convenience.

jieyouxu · 2024-04-23T17:37:39Z

I found that dissimilar only allows comparison based on char tokens, whereas diff should ideally perform comparisons starting with lines as tokens.

If that's the case, then I think using similiar is fine because otherwise you'd be basically reimplementing it on top of dissimiliar (I had a brief look, and it looks like dissimiliar didn't port over google's Diff Match Patch's line mode).

klensy · 2024-04-23T20:10:12Z

Well, i guess diff won't work too?

jieyouxu · 2024-04-24T20:02:24Z

Cargo.lock

@@ -3343,6 +3343,7 @@ version = "0.0.0"
 dependencies = [
 "object 0.34.0",
 "regex",
+ "similar",


~~Could you check if you can just use the diff from the environment? As in, I think we already assume diff exists:~~

rust/tests/run-make/tools.mk

Lines 21 to 22 in 7bb4f08

# diff with common flags for multi-platform diffs against text output

DIFF := diff -u --strip-trailing-cr

~~If that's not available, then I'm open to adding similar.~~

EDIT: Now that I think about it, the easier to run the test the better. For example, I think grep isn't by default available on Windows? Would be nice to not have to rely on those external dependencies if possible/reasonable especially if they can have platform-specific differences and what not.

Would be nice to not have to rely on those external dependencies if possible/reasonable especially if they can have platform-specific differences and what not.

I think so too.

jieyouxu · 2024-04-24T20:24:29Z

tests/run-make/no-input-file/rmake.rs

+use run_make_support::{diff, rustc};
+
+fn main() {
+    let output = rustc().arg("--print").arg("crate-name").run_fail_assert_exit_code(1);


One last nit: can you add a print helper method to Rustc? So the callsite becomes something like

rustc().print("crate-name")

This PR looks good to me otherwise.

jieyouxu · 2024-04-25T01:52:54Z

Thanks for the PR!

@bors r+ rollup

bors · 2024-04-25T01:52:56Z

📌 Commit f3530cf has been approved by jieyouxu

It is now in the queue for this repository.

Rewrite the `no-input-file.stderr` test in Rust and support diff Rewrite the `no-input-file.stderr` test from rust-lang#121876. Use the `similar` lib to replace the `diff` command.

…iaskrgr Rollup of 4 pull requests Successful merges: - rust-lang#124257 (Rewrite the `no-input-file.stderr` test in Rust and support diff) - rust-lang#124324 (Minor AST cleanups) - rust-lang#124327 (CI: implement job skipping in Python matrix calculation) - rust-lang#124345 (Enable `--check-cfg` by default in UI tests) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 3 pull requests Successful merges: - rust-lang#124257 (Rewrite the `no-input-file.stderr` test in Rust and support diff) - rust-lang#124324 (Minor AST cleanups) - rust-lang#124327 (CI: implement job skipping in Python matrix calculation) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#124257 - JoverZhang:rmake-diff, r=jieyouxu Rewrite the `no-input-file.stderr` test in Rust and support diff Rewrite the `no-input-file.stderr` test from rust-lang#121876. Use the `similar` lib to replace the `diff` command.

rustbot assigned jieyouxu Apr 22, 2024

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 22, 2024

JoverZhang commented Apr 22, 2024

View reviewed changes

JoverZhang marked this pull request as ready for review April 22, 2024 13:33

jieyouxu requested changes Apr 22, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 22, 2024

This comment was marked as resolved.

Sign in to view

JoverZhang force-pushed the rmake-diff branch from be5e647 to aae4ebc Compare April 24, 2024 14:41

JoverZhang requested a review from jieyouxu April 24, 2024 14:42

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 24, 2024

jieyouxu reviewed Apr 24, 2024

View reviewed changes

Rewrite no-input-file.stderr test in Rust and support diff

f3530cf

JoverZhang force-pushed the rmake-diff branch from aae4ebc to f3530cf Compare April 25, 2024 01:13

JoverZhang requested a review from jieyouxu April 25, 2024 01:17

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 25, 2024

matthiaskrgr mentioned this pull request Apr 25, 2024

Rollup of 4 pull requests #124358

Closed

matthiaskrgr mentioned this pull request Apr 25, 2024

Rollup of 3 pull requests #124360

Merged

bors merged commit a2d6b1b into rust-lang:master Apr 25, 2024
10 checks passed

rustbot added this to the 1.79.0 milestone Apr 25, 2024

JoverZhang deleted the rmake-diff branch April 26, 2024 01:13

jieyouxu mentioned this pull request Jun 9, 2024

Tracking Issue for porting run-make tests to use Rust #121876

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite the `no-input-file.stderr` test in Rust and support diff #124257

Rewrite the `no-input-file.stderr` test in Rust and support diff #124257

JoverZhang commented Apr 22, 2024

rustbot commented Apr 22, 2024

JoverZhang Apr 22, 2024

jieyouxu Apr 22, 2024

jieyouxu Apr 22, 2024

rustbot commented Apr 22, 2024

klensy commented Apr 22, 2024

jieyouxu left a comment

jieyouxu Apr 22, 2024

jieyouxu Apr 22, 2024

JoverZhang commented Apr 23, 2024

jieyouxu commented Apr 23, 2024

klensy commented Apr 23, 2024

This comment was marked as resolved.

jieyouxu Apr 24, 2024 •

edited

Loading

JoverZhang Apr 25, 2024

jieyouxu Apr 24, 2024

jieyouxu commented Apr 25, 2024

bors commented Apr 25, 2024

	# diff with common flags for multi-platform diffs against text output
	DIFF := diff -u --strip-trailing-cr

Rewrite the no-input-file.stderr test in Rust and support diff #124257

Rewrite the no-input-file.stderr test in Rust and support diff #124257

Conversation

JoverZhang commented Apr 22, 2024

rustbot commented Apr 22, 2024

JoverZhang Apr 22, 2024

Choose a reason for hiding this comment

jieyouxu Apr 22, 2024

Choose a reason for hiding this comment

jieyouxu Apr 22, 2024

Choose a reason for hiding this comment

rustbot commented Apr 22, 2024

klensy commented Apr 22, 2024

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu Apr 22, 2024

Choose a reason for hiding this comment

jieyouxu Apr 22, 2024

Choose a reason for hiding this comment

JoverZhang commented Apr 23, 2024

jieyouxu commented Apr 23, 2024

klensy commented Apr 23, 2024

This comment was marked as resolved.

jieyouxu Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

JoverZhang Apr 25, 2024

Choose a reason for hiding this comment

jieyouxu Apr 24, 2024

Choose a reason for hiding this comment

jieyouxu commented Apr 25, 2024

bors commented Apr 25, 2024

Rewrite the `no-input-file.stderr` test in Rust and support diff #124257

Rewrite the `no-input-file.stderr` test in Rust and support diff #124257

jieyouxu Apr 24, 2024 •

edited

Loading