8000 Fix #387: Add unit test for opening and writing to a file using the Promise API by dnguneratne · Pull Request #397 · filerjs/filer · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix #387: Add unit test for opening and writing to a file using the Promise API #397

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix #387: Add unit test for opening and writing to a file using the Promise API #397

wants to merge 2 commits into from

Conversation

dnguneratne
Copy link

This pull request adds a new test to ensure that,

  • a file was opened,
  • a buffer of 8 numbers were successfully written to it,
  • and check if the file is of type FILE,

using the Promise API.

0xazure and others added 2 commits September 12, 2018 21:43
@0xazure @humphd
Update karma dev dependency
fd3de6b 
Resolves `npm audit` warnings about 1 low, 5
moderate vulnerabilities related to versions before
karma@3.0.0.

Closes #384.
@dnguneratne
#387 Added unit test for writing data using the Promise API
ad42a0d
@seanprashad
Copy link
Contributor
seanprashad commented Sep 18, 2018 •
edited
Loading

Hey @dnguneratne - nice work 🎉 🎉

I have one suggestion that would help move this along faster - it looks like you need to rebase as @0xazure's commit was landed recently.

I think git checkout master && git pull upstream master && git checkout issue-387 && git rebase master would be what we're looking for. Incorrect commands - looking into it

@dnguneratne
Copy link
Author

I forked this repo today and branched off master. So if I'm not mistaken it should include @0xazure's commit. However, I know that the develop branch is behind. I'm not sure if this PR should be to develop or master, especially because of #388. @humphd?

@0xazure
Copy link
Contributor
0xazure commented Sep 18, 2018

Per #383 it looks like PRs should target master, though #388 brings up a great point about how pre-built versions of Filer are distributed from GitHub; @humphd should we be re-building dist/ and including those changes (if any) in future PRs?

@humphd
Copy link
Contributor
humphd commented Sep 18, 2018

We haven't had a good automated release story for what's in dist/ and what's on npm. That needs to happen. For the moment, there's going to be a ton of churn on master over the coming few weeks as we land tests and other student PRs, so I'm not too worried about things being out of sync. When you run npm test it will rebuild, so it will always be fresh.

Finally, a word about develop vs. master. I recently switched the default branch of Filer to master from develop so as to not confuse new git users. However, it may have messed up some people who might have been on develop.

I'm not going to stress too much about rebasing in this first release for all students; if you know how to do it, and are comfortable, go for it. But if not, we'll just merge and deal with rebasing later.

@humphd
Copy link
Contributor
humphd commented Sep 18, 2018

Filed #404 to figure out dist/

@humphd
Copy link
Contributor
humphd commented Sep 18, 2018

To fix this PR, do the following (assuming filerjs/filer is your upstream remote):

$ git fetch upstream
$ git checkout -B master upstream/master
$ git checkout issue-387
$ git rebase master
...assuming this works without issue...
$ git push origin issue-387 -f

If that doesn't work (I haven't tested it), let me know and I'll help debug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dnguneratne @seanprashad @0xazure @humphd
0