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

Test coverage for GitTempDir #1901

Merged
merged 1 commit into from Jan 11, 2019

Conversation

2 participants
@smashwilson
Copy link
Member

smashwilson commented Jan 10, 2019

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • Suggestion: You can use checklists to keep track of progress for the sections on metrics, tests, documentation, and user research.

Description of the Change

Adds test coverage for GitTempDir to prevent coverage flapping in unrelated pull requests.

Alternate Designs

I could have only covered the lines that weren't hit by other codepaths, but it's a small class so I just covered the whole thing.

Benefits

GitTempDir is one of the culprits in test coverage flapping that we see on unrelated PRs. This is one step toward minimizing those changes and keeping CodeCov output relevent.

Possible Drawbacks

N/A

Applicable Issues

N/A

Metrics

N/A

Tests

This should raise GitTempDir's coverage to 100%.

Documentation

N/A

Release Notes

N/A

User Experience Research (Optional)

N/A

@smashwilson smashwilson added this to In Progress 🔧 in Sprint : 9 January 2019 - 12 February 2019 : v0.25.0 via automation Jan 10, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 10, 2019

Codecov Report

Merging #1901 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1901      +/-   ##
==========================================
- Coverage   91.09%   91.05%   -0.05%     
==========================================
  Files         185      185              
  Lines       10719    10719              
  Branches     1575     1575              
==========================================
- Hits         9765     9760       -5     
- Misses        954      959       +5
Impacted Files Coverage Δ
lib/models/conflicts/side.js 92.18% <0%> (-4.69%) ⬇️
lib/atom/decoration.js 84.33% <0%> (-2.41%) ⬇️
lib/controllers/editor-conflict-controller.js 94.94% <0%> (-1.02%) ⬇️
lib/git-temp-dir.js 100% <0%> (+3.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4786390...9658e1e. Read the comment docs.

@smashwilson smashwilson requested a review from atom/github-package Jan 10, 2019

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 10, 2019

Looks like we're good 👌

Sprint : 9 January 2019 - 12 February 2019 : v0.25.0 automation moved this from In Progress 🔧 to QA Review 🔬 Jan 10, 2019

@annthurium
Copy link
Contributor

annthurium left a comment

thanks for making our test coverage more stable! I have a question or two for my own education but nothing blocking.

Why were these files causing flapping in the first place? Were there timing issues or something, where sometimes we met certain conditions and sometimes not?

}

await tempDir.ensure();
assert.strictEqual(root, tempDir.getRootPath());

This comment has been minimized.

@annthurium

annthurium Jan 10, 2019

Contributor

just for my own education...

you assign root = tempDir.getRootPath on line 11, and then assert that that these are still equal. With this assertion, are you testing that ensure() does not change the root path?

This comment has been minimized.

@smashwilson

smashwilson Jan 11, 2019

Author Member

With this assertion, are you testing that ensure() does not change the root path?

Yup, exactly. I wanted to assert that multiple calls to ensure() wouldn't create new temp directories, basically.

const tempDir = new GitTempDir();
await tempDir.ensure();

assert.match(tempDir.getSocketPath(), /^\\\\\?\\pipe\\/);

This comment has been minimized.

@annthurium

annthurium Jan 10, 2019

Contributor

fun regex. oh, windows.

This comment has been minimized.

@smashwilson

smashwilson Jan 11, 2019

Author Member

Named pipes are fun 🙃

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 11, 2019

Why were these files causing flapping in the first place? Were there timing issues or something, where sometimes we met certain conditions and sometimes not?

GitTempDir was only tested incidentally through GitShellOutStrategy and Repository tests, so we had a few lines that weren't being explicitly exercised. Getting us to full coverage with an explicit test is the best way to make sure these don't fall through the cracks.

@smashwilson smashwilson merged commit 4316bb8 into master Jan 11, 2019

2 checks passed

codecov/patch Coverage not affected when comparing 4786390...9658e1e
Details
codecov/project 91.05% (-0.05%) compared to 4786390
Details

Sprint : 9 January 2019 - 12 February 2019 : v0.25.0 automation moved this from QA Review 🔬 to Merged ☑️ Jan 11, 2019

@smashwilson smashwilson deleted the aw/coverage/git-temp-dir branch Jan 11, 2019

@smashwilson smashwilson referenced this pull request Jan 11, 2019

Merged

Correct remaining flapping test coverage #1903

4 of 4 tasks complete

@kuychaco kuychaco referenced this pull request Feb 4, 2019

Closed

v0.25.0-0 QA Review #1936

14 of 18 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.