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: add test for failed save in REPL #1818

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 28, 2015

Getting rid of a TODO comment (#264)

@Trott Trott force-pushed the test-failed-save branch 2 times, most recently from c1ab67f to c373204 Compare May 28, 2015 02:49
@mscdex mscdex added the test Issues and PRs related to the tests. label May 28, 2015
@Fishrock123
Copy link
Contributor

r=@chrisdickinson

@mscdex mscdex added the repl Issues and PRs related to the REPL subsystem. label Jun 8, 2015
@Trott
Copy link
Member Author

Trott commented Jun 11, 2015

Can someone throw this into Jenkins to make sure it actually works for all the operating systems? It should, but uh...you know...

// make sure I get a failed to save message and not some other error
assert.equal(data, 'Failed to save:' + invalidFileName + '\n');
// reset to no-op
putIn.write = function() {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

We want to test that when the file can't be saved, the REPL reports that to the user rather than (say) failing silently.

Copy link
Member Author

Choose a reason for hiding this comment

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

It might be clearer and more robust to override fs.writeFileSync() or whatever so that it triggers an error no matter what. Downside is that ties the test to the REPL implementation a little more closely than one might like.

@Trott
Copy link
Member Author

Trott commented Jun 11, 2015

Pushed commit to change var to const per suggestion from @thefourtheye

(Unfortunately, the rebase to master means that test/parallel/test-cluster-worker-wait-server-close.js fails. When that gets resolved in master, I can rebase again.)

@Trott
Copy link
Member Author

Trott commented Jun 12, 2015

Rebased to get test fix, runs clean now, woot.

@chrisdickinson
Copy link
Contributor

@chrisdickinson
Copy link
Contributor

LGTM if the tests pass.

Trott added a commit that referenced this pull request Jun 13, 2015
PR-URL: #1818
Reviewed-By: Chris Dickinson <[email protected]>
@Trott
Copy link
Member Author

Trott commented Jun 13, 2015

Merged in 8ea6844

@Trott Trott closed this Jun 13, 2015
@Trott Trott deleted the test-failed-save branch January 9, 2022 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants