Skip to content

Make run-pass/env-home-dir.rs test more robust - #47718

Merged
bors merged 2 commits into
rust-lang:masterfrom
malbarbo:env-home-dir
Jan 30, 2018
Merged

Make run-pass/env-home-dir.rs test more robust#47718
bors merged 2 commits into
rust-lang:masterfrom
malbarbo:env-home-dir

Conversation

@malbarbo

Copy link
Copy Markdown
Contributor

Remove the assumption that home_dir always returns Some.

This allows the test to be executed with cross.

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @nikomatsakis

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

Comment threadsrc/test/run-pass/env-home-dir.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change to now construct a relative path instead of some random string?

It seems like before we never even looked at the value we got back, is that right?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could have only changed the line that assume the home_dir returns Some, that is

- assert!(home_dir().is_some());+ assert_ne!(home_dir(), Some(PathBuf::from("/home/MountainView")));

but that could fail if getpwuid_r returns "/home/MountainView" (or other chosen string). Setting newhome to a relative path avoids this situation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're saying that could fail if set_var had no effect, but it just happens that the directory name is /home/MountainView? I suppose that's true. It just seems a bit weird for the windows test (below) to be asymmetric.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I don't quite understand what is being fixed here =)

We're still asserting that home_dir returns Some -- how was the test failing before?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Anyway, the patch seems fine, just trying to understand. =)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're saying that could fail if set_var had no effect, but it just happens that the directory name is /home/MountainView? I suppose that's true. It just seems a bit weird for the windows test (below) to be asymmetric.

I changed the unix case because that's was failing for me, but in the end, I just need that the test does not assume home_dir return Some.

We're still asserting that home_dir returns Some -- how was the test failing before?

The test was failing because home_dir was returning None. Now we are using assert_ne!, so it pass if home_dir returns None.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed the assert_ne thing somehow. Can you add a comment on top saying something like:

/// When HOME is not set, some platforms return `None`, but others return `Some` with a default.
/// Just check that it is not the `newhome_path`.

Also, I think it'd be best if the unix + windows tests match up -- I'm ok with either your new version or the old one, but I guess it's easier to just revert those commits.

@nikomatsakisnikomatsakis added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 25, 2018
Remove the assumption that home_dir always returns Some
This allows the test to be executed with
[cross](https://github.com/japaric/cross).
@malbarbo

Copy link
Copy Markdown
ContributorAuthor

Updated.

@nikomatsakis

Copy link
Copy Markdown
Contributor

@bors r+ -- thanks!

@bors

bors commented Jan 29, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit adeb0ae has been approved by nikomatsakis

@kennytm

Copy link
Copy Markdown
Member

@bors rollup

kennytm added a commit to kennytm/rust that referenced this pull request Jan 29, 2018
Make run-pass/env-home-dir.rs test more robust
Remove the assumption that home_dir always returns Some.
This allows the test to be executed with [cross](https://github.com/japaric/cross).
@kennytmkennytm mentioned this pull request Jan 29, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Jan 30, 2018
Make run-pass/env-home-dir.rs test more robust
Remove the assumption that home_dir always returns Some.
This allows the test to be executed with [cross](https://github.com/japaric/cross).
@kennytmkennytm mentioned this pull request Jan 30, 2018
bors added a commit that referenced this pull request Jan 30, 2018
Rollup of 12 pull requests
- Successful merges: #47515, #47603, #47718, #47732, #47760, #47780, #47822, #47826, #47836, #47839, #47853, #47855
- Failed merges:
@bors
bors merged commit adeb0ae into rust-lang:masterJan 30, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@malbarbo@rust-highfive@nikomatsakis@bors@kennytm