Skip to content

feat: Initial test case for repl errors #17

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

antsmartian
Copy link
Contributor

@antsmartian antsmartian commented Jul 28, 2018

Hey @devsnek

I worked on the item : #6

This should be the beginning for adding test cases for repl 👍

Have a look and let me know we can follow the same pattern for other test cases too. Once this is good, may be we can then add scripts to run our tests on our npm build process.

Thanks for your time on this.


new REPL(inoutStream, inoutStream);

const errorMessage = 'SyntaxError: Invalid or unexpected token';
Copy link
Member

Choose a reason for hiding this comment

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

tests shouldn't use specific messages because they change based on which engine node runs in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm, then how can we assert the expected value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ping @devsnek

Copy link
Member

Choose a reason for hiding this comment

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

take a look at the tests for repl in node core

@antsmartian
Copy link
Contributor Author

@devsnek Thanks for your help. I have refactored the assertion accordingly. Let me know if anything needs to be done further.

@antsmartian
Copy link
Contributor Author

ping @devsnek

1 similar comment
@antsmartian
Copy link
Contributor Author

ping @devsnek

@devsnek
Copy link
Member

devsnek commented Aug 21, 2018

i'd like to see if we can set up CI of some sort for the linter before we merge more PRs

@antsmartian
Copy link
Contributor Author

@devsnek Perfect! Sounds good to me.

@devsnek devsnek force-pushed the master branch 3 times, most recently from 371bc3d to dc3e899 Compare September 1, 2018 00:31
@devsnek devsnek force-pushed the master branch 2 times, most recently from 2f98d80 to ef458c1 Compare September 28, 2018 13:52
@antsmartian
Copy link
Contributor Author

@devsnek Any update on this?

@devsnek devsnek force-pushed the master branch 2 times, most recently from 69006e9 to a8fd2b0 Compare November 2, 2018 04:28
@antsmartian
Copy link
Contributor Author

I know it's a long time, any update here? cc @devsnek

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.

2 participants