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

Rename jasmine_node_test to nodejs_jasmine_test #394

Closed
haikalpribadi opened this issue Oct 26, 2018 · 4 comments
Closed

Rename jasmine_node_test to nodejs_jasmine_test #394

haikalpribadi opened this issue Oct 26, 2018 · 4 comments

Comments

@haikalpribadi
Copy link

Hi, as we can see, the naming convention for the rules in rules_nodejs starts with nodejs_<...> (such as nodejs_binary and nodejs_test. However, jasmine_node_test breaks this convention. Before rule names starts getting messier, can we rename jasmine_node_test to nodejs_jasmine_test to maintain consistency?

@haikalpribadi
Copy link
Author

We can make a PR for this if the @bazelbuild team thinks it makes sense. cc: @vmax

@alexeagle
Copy link
Collaborator

Actually we discussed last week in the context of #168 that this rule shouldn't have jasmine in the name. We don't expose which test runner is used, and this rule could be used to run most node unit tests.

So I think the new name should be nodejs_unit_test
/cc @vikerman

We're happy to take a PR for that. The tricky bit will be how to roll this out without breaking users. Probably needs to be available under both names during a transition period of several releases.

@haikalpribadi
Copy link
Author

Well, I would love just see one test rule, such as nodejs_unit_test. However, from what I understand is that there are 3 unit test framework that people use for NodeJS. At @graknlabs we use Jest. So we're (@vmax) also making a PR to add nodejs_jest_test. So that means we can just have Jasmine to be the default nodejs_unit_test, unless it becomes a higher level rule to wrap all 3 test framework (or any others) and have it accept an argument to select the framework to use. What do you think @alexeagle ?

@alexeagle
Copy link
Collaborator

Actually this one is named "jasmine_node_test" to match the rule we use internally at Google, so I don't think it's worth renaming it. I do agree that we should have mocha_test and jest_test rules.

alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue Oct 17, 2020
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants