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

Fix issue #681: Updated filer/tests/spec/filer.filesystem.spec.js #695

Merged
merged 3 commits into from
Feb 1, 2019

Conversation

woosle1234
Copy link
Contributor

require lines use const instead of var
variable declaration in local scopes use let instead of var

@codecov-io
Copy link

Codecov Report

Merging #695 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #695   +/-   ##
=======================================
  Coverage   86.71%   86.71%           
=======================================
  Files          16       16           
  Lines        1746     1746           
=======================================
  Hits         1514     1514           
  Misses        232      232

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 26b47ee...aa70c90. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Jan 29, 2019

Codecov Report

Merging #695 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #695   +/-   ##
=======================================
  Coverage   86.71%   86.71%           
=======================================
  Files          16       16           
  Lines        1746     1746           
=======================================
  Hits         1514     1514           
  Misses        232      232
Impacted Files Coverage Δ
src/shell/environment.js 100% <0%> (ø) ⬆️

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 26b47ee...fdec4b1. Read the comment docs.

@ithompson4
Copy link
Contributor

Hi, I think you are missing 'use strict' at the top of your file.

@woosle1234
Copy link
Contributor Author

Hi, I think you are missing 'use strict' at the top of your file.

Added

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

This is coming along well. There are two main things we need to do to finish it.

  1. I think you've gotten confused about what's required for tests with a bug. You've added tests/bugs/issue681.js, which isn't actually necessary in this case. Sometimes we do this (i.e., add a test for a particular bug), because we are fixing a problem, and want to add a test to make sure it doesn't get added back again. The tests in tests/bugs/* are all specific to old bugs we don't want to regress. I've left some comments for how to undo this.

  2. You need to make some changes to use const in the final file. What you did is actually correct, but you can use const because those variables won't every change.

tests/index.js Outdated Show resolved Hide resolved
tests/spec/filer.filesystem.spec.js Outdated Show resolved Hide resolved
tests/spec/filer.filesystem.spec.js Outdated Show resolved Hide resolved
tests/spec/filer.filesystem.spec.js Outdated Show resolved Hide resolved
tests/spec/filer.filesystem.spec.js Outdated Show resolved Hide resolved
@woosle1234
Copy link
Contributor Author

This is coming along well. There are two main things we need to do to finish it.

  1. I think you've gotten confused about what's required for tests with a bug. You've added tests/bugs/issue681.js, which isn't actually necessary in this case. Sometimes we do this (i.e., add a test for a particular bug), because we are fixing a problem, and want to add a test to make sure it doesn't get added back again. The tests in tests/bugs/* are all specific to old bugs we don't want to regress. I've left some comments for how to undo this.
  2. You need to make some changes to use const in the final file. What you did is actually correct, but you can use const because those variables won't every change.

Changes implemented in commit "issue 681 revision v2"

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Great!

@humphd humphd merged commit 8eaaeee into filerjs:master Feb 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants