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

make ui test output patch compatible #41948 #42144

Merged
merged 1 commit into from
May 23, 2017
Merged

make ui test output patch compatible #41948 #42144

merged 1 commit into from
May 23, 2017

Conversation

cengiz-io
Copy link
Contributor

@cengiz-io cengiz-io commented May 21, 2017

Hello!

Previously with #41474 I've changed the internals of UI test output comparison mechanism.

That change didn't change the diff format that we were producing but we needed to improve it anyway.

This makes unified diff lines a little bit more patch compatible.

Also I tried to introduce a unit test to check this but couldn't decide which of the following to implement:

  1. Should I replace println macros with Writers? And access the produced output within a test?
  2. Should I add an external test (something like src/test/run-pass/command-exec.rs)
  3. There are crates that capture stdout. Are they safe to use here? (I don't think so)

Thanks!

cc @nikomatsakis

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@carols10cents
Copy link
Member

r? @nikomatsakis

Niko, could you take a look at @cengizio's questions about unit tests?

@rust-highfive rust-highfive assigned nikomatsakis and unassigned aturon May 22, 2017
@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2017
@nikomatsakis
Copy link
Contributor

Yeah, I'm not sure what's the best way to handle unit-testing. The most "robust" would be to somehow "execute" the runtest tool and capture the output, but perhaps it'd be easier to just isolate the routines in question and have them read from simulated data structures, writing to a Writer. In any case, I don't think it needs to block this PR -- we've come this far without unit-testing compiletest. It may not even be worth the trouble.

@nikomatsakis
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 22, 2017

📌 Commit 9111d07 has been approved by nikomatsakis

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 23, 2017
make ui test output patch compatible rust-lang#41948

Hello!

Previously with rust-lang#41474 I've changed the internals of UI test output comparison mechanism.

That change didn't change the diff format that we were producing but we needed to improve it anyway.

This makes unified diff lines a little bit more `patch` compatible.

Also I tried to introduce a unit test to check this but couldn't decide which of the following to implement:

1. Should I replace `println` macros with `Writer`s? And access the produced output within a test?
2. Should I add an external test (something like `src/test/run-pass/command-exec.rs`)
3. There are crates that capture `stdout`. Are they safe to use here? (I don't think so)

Thanks!

cc @nikomatsakis
bors added a commit that referenced this pull request May 23, 2017
Rollup of 8 pull requests

- Successful merges: #42016, #42122, #42144, #42145, #42151, #42152, #42160, #42163
- Failed merges:
@bors bors merged commit 9111d07 into rust-lang:master May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants