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 #500: add test for fs.open to check flags #507

Closed

Conversation

Projects
None yet
3 participants
@Mera-Gangapersaud
Copy link
Contributor

commented Sep 24, 2018

Added test for fs.open to check that flags are valid per issue #500

@SeanPrashad

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

👏🏽 This 👏🏽 is 👏🏽 some 👏🏽 good 👏🏽 stuff 👏🏽 @Mera-Gangapersaud!

It looks like we have one nit (nit-pick, ie. something small) to fix up according to TravisCI:

https://travis-ci.org/filerjs/filer/builds/432704203#L2181

When you have a chance, feel free to fix the linting issue. Then you can either add a new commit with a message of something like Correct TravisCI linting error, or (what I would do) append your fix to your previous commit. This will essentially tack that one linting fix onto your existing commit (79afd62) and it'll trigger TravisCI to rerun!

To do the second method (which will keep a cleaner commit history and I know you're a perfectionist), you'll need to:

(1) Correct the linting issue in your editor of choice
(2) Enter git status in your terminal/command prompt and verify that single file was the only thing that changed
(3) Enter git add fileThatIModifiedToFixTravis to add the file to staging
(4) Enter git commit --amend to add that fix to the last commit
(5) Lastly, enter git push -f to force push to your branch (we need to do this as the commit hash changed from 79afd... to the new one you'll see)

And voila! 🎉

Give me a shout if you need any help with this!

@SeanPrashad

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

Also, we'll need to include the original issue number (#500) in either the description, or commit message so that it automatically closes the respective issue!

@Mera-Gangapersaud Mera-Gangapersaud force-pushed the Mera-Gangapersaud:issue#500 branch 2 times, most recently from 80d4406 to 3756903 Sep 28, 2018

@Mera-Gangapersaud Mera-Gangapersaud force-pushed the Mera-Gangapersaud:issue#500 branch from 3756903 to 9f754fb Sep 28, 2018

@SeanPrashad

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2018

I see the linting issue is fixed - nice work!!

We now have a new problem (that isn't soo bad..). Since we amended that linting fix to the previous commit, you did a good thing and updated the commit message. However, this over wrote the old commit message, which was more descriptive and what we were looking for.

Can you run git commit --amend and update it to be something like the original message you had? Maybe something like Fixes #500: Add a test for fs.open() when flag is invalid

@@ -136,4 +136,13 @@ describe('fs.open', function() {
});
});
});

it('should return an error when flag in invalid', function(done) {

This comment has been minimized.

Copy link
@humphd

humphd Oct 9, 2018

Contributor

Let's change return in this to throw

var fs = util.fs();

expect(fs.open('/myfile', 'abcd', function(){
done();

This comment has been minimized.

Copy link
@humphd

humphd Oct 9, 2018

Contributor

I don't think we need done at all in this test, since nothing async is happening (i.e., it throws right away

it('should return an error when flag in invalid', function(done) {
var fs = util.fs();

expect(fs.open('/myfile', 'abcd', function(){

This comment has been minimized.

Copy link
@humphd

humphd Oct 9, 2018

Contributor

Usually you can't pass a function that will throw like this, you have to wrap it in a function that the test runner can call:

expect(function() {
  fs.open('/myfile', 'abcd', function(){});
}).to.throw('flags is not valid');

It's sort of like having a fuse on a bomb: before you set a bomb off, you need to be far away. By wrapping the function in an outer function, you can safely call the outer function, knowing that it will likely throw.

@humphd

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

I've made the changes above, rebased, and in doing so, discovered that we have a bug that this test revealed. Nice work @Mera-Gangapersaud. I added a fix in 97d2d1b. Your code is landed now.

@humphd humphd closed this Dec 13, 2018

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.