Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Conversation

@smashwilson
Copy link
Contributor

@smashwilson smashwilson commented Jan 11, 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

I'm adding unit tests to cover the remaining lines that I've seen flap in our CodeCov reports, either by adding tests for specific lines I've seen flap, or just explicitly covering entire files and were only covered incidentally before.

Specifically:

  • lib/models/conflicts/side.js
  • lib/atom/decoration.js
  • lib/controllers/editor-conflict-controller.js
  • lib/models/repository-states/present.js

Alternate Designs

N/A

Benefits

More reliable and actionable CodeCov reports.

Possible Drawbacks

It's a fair amount of rote work to do. But my head is thoroughly stuffed today so this is a good way to deliver some value without requiring higher-order thinking skills.

Applicable Issues

Continuing the work begin in #1900 and #1901.

Metrics

The CodeCov master report is a good one to watch.

Tests

Ideally we'll see coverage ⬆️ in the CodeCov report comment for those files. (Note that there is a delay while the Windows tests roll in.)

Documentation

N/A

Release Notes

N/A

User Experience Research (Optional)

N/A

@codecov
Copy link

codecov bot commented Jan 11, 2019

Codecov Report

Merging #1903 into master will increase coverage by 0.51%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1903      +/-   ##
==========================================
+ Coverage   91.04%   91.55%   +0.51%     
==========================================
  Files         185      185              
  Lines       10719    10707      -12     
  Branches     1575     1574       -1     
==========================================
+ Hits         9759     9803      +44     
+ Misses        960      904      -56
Impacted Files Coverage Δ
lib/models/conflicts/banner.js 100% <100%> (+16.66%) ⬆️
lib/controllers/editor-conflict-controller.js 100% <100%> (+5.05%) ⬆️
lib/models/conflicts/side.js 100% <100%> (+7.81%) ⬆️
lib/models/repository-states/present.js 100% <100%> (+4.97%) ⬆️
lib/atom/decoration.js 100% <100%> (+15.66%) ⬆️
lib/git-shell-out-strategy.js 88.41% <0%> (+0.71%) ⬆️

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 4316bb8...a8a5d61. Read the comment docs.

@smashwilson smashwilson requested a review from a team January 12, 2019 17:03
componentDidUpdate(prevProps) {
if (this.props.editorHolder !== prevProps.editorHolder) {
this.editorSub.dispose();
this.editorSub = this.state.editorHolder.observe(this.observeParents);

Choose a reason for hiding this comment

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

did you change from state to props because that's easier to test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it was actually just broken before 😅 We hadn't had any tests that covered an update of an existing component, and apparently it doesn't come up in our package yet. But editorHolder is a member of the state in the wrapper class that reads Context, not in the BareDecoration!

const bundle = await this.git().getStatusBundle();
const results = await this.formatChangedFiles(bundle);
results.branch = bundle.branch;
if (!results.branch.aheadBehind) {

Choose a reason for hiding this comment

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

this is unused, I take it? good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! getStatusBundle will always return an empty aheadBehind with our versions of git and what-the-status.

await assert.async.isTrue(refreshResolutionProgress.calledWith(fixtureFile));
});

it('performs a resolution from the context menu', function() {

Choose a reason for hiding this comment

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

oh cool - I did not know resolving from the context menu was a thing.

@smashwilson smashwilson merged commit d8f360e into master Jan 14, 2019
@smashwilson smashwilson deleted the aw/coverage/misc branch January 14, 2019 18:59
@kuychaco kuychaco mentioned this pull request Feb 4, 2019
18 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants