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

Make test runners available from `yarn run test:*` #26671

Merged
merged 3 commits into from Jan 3, 2019

Conversation

Projects
None yet
6 participants
@mattkime
Copy link
Contributor

mattkime commented Dec 4, 2018

Summary

Finding the correct test runner is a bit indirect between Jest and Mocha, x-pack code and the rest. This makes testing functionality more visible since its available via yarn run and it provides the same interface for different tests.

@spalger

This comment has been minimized.

Copy link
Member

spalger commented Dec 4, 2018

I think the confusion is caused by us only half-way implementing #11095. I would personally prefer if we added node scripts/ for each of the test styles rather than adding more yarn scripts...

@mattkime

This comment has been minimized.

Copy link
Contributor

mattkime commented Dec 4, 2018

@spalger I guess I'm a little disappointed to see that we're choosing a non standard route.

@cjcenizal

This comment has been minimized.

Copy link
Contributor

cjcenizal commented Dec 4, 2018

I have nothing against executing tests via node scripts, but discoverability nears zero when we don't list these scripts in package.json. Having everything listed under scripts in package.json gives me a single place for me to see what I can run, we can use naming patterns to group things (e.g. test: prefixes shows me at a glance which kinds of tests I can execute), and Matt's example of using a comment to describe a script really helps explain the role of each script to me. As Matt points out, it's also a common convention that most JS developers are familiar with.

Can I suggest that we create parity between all node scripts and package.json scripts by creating a script alias for each existing node script, and as we add node scripts in the future we add a script alias to package.json?

@mattapperson

This comment has been minimized.

Copy link
Member

mattapperson commented Dec 4, 2018

I would also add that integration with things like VSCode for automating test running is gained very easily with npm scripts. Just a developer DX consideration 😊

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Dec 4, 2018

@@ -13,6 +13,8 @@
"test": "gulp test",
"test:browser:dev": "gulp testbrowser-dev",
"test:browser": "gulp testbrowser",
"test:jest": "node scripts/jest",
"test:mocha": "grunt test:server #server means tests run in node, not browser, poorly named",

This comment has been minimized.

@tylersmalley

tylersmalley Dec 27, 2018

Member

If this is poorly named, how about renaming test:server to test:mocha?

@mattapperson

This comment has been minimized.

Copy link
Member

mattapperson commented Dec 27, 2018

// cc @silne30

@tylersmalley

This comment has been minimized.

Copy link
Member

tylersmalley commented Dec 27, 2018

Is the problem here primarily documentation? It sounds like having them scripts defined in the package.json is a form of documentation and discoverability.

I am fine with CJ's suggestion of creating aliases for the node scripts, but I don't think we need to have parity with each one. There are some which aren't common enough to justify clobbering run scripts and therefore reducing the discoverability. We should really be focused on improving the documentation and not relying on something like package.json where comments are not even supported.

@@ -41,6 +41,8 @@
"test:dev": "grunt test:dev",
"test:quick": "grunt test:quick",
"test:browser": "grunt test:browser",
"test:jest": "node scripts/jest",
"test:mocha": "grunt test:server #server means tests run in node, not browser, poorly named",

This comment has been minimized.

@tylersmalley

tylersmalley Dec 27, 2018

Member

I am not sure how I feel about the hack here for comments.

@mattapperson

This comment has been minimized.

Copy link
Member

mattapperson commented Dec 27, 2018

I think the issue here is a matter of package.json being a standard. Needing to have docs around a nonstandard way of doing things is fine so long as there in value in doing things a non-standard way.
As for not needing parity... then a choice needs to be made for each script, one often based on a given persons opinion and perspective at that moment. I think exposing things here is better. If it's too much noise, perhaps that's a sort of code smell of our systems, not of the exposing of them? Just thinking out loud here 😊

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Dec 27, 2018

@mattkime

This comment has been minimized.

Copy link
Contributor

mattkime commented Dec 27, 2018

@tylersmalley I think I've addressed your concerns.

@mattkime mattkime added the :Dev Tools label Dec 27, 2018

@mattkime mattkime merged commit 5f8d53c into elastic:master Jan 3, 2019

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
kibana-ci Build finished.
Details

mattkime added a commit to mattkime/kibana that referenced this pull request Jan 3, 2019

Make test runners available from `yarn run test:*` (elastic#26671)
* yarn test:mocha yarn test:jest, x-pack too

* remove inline comments, deprecate test:server and replace with test:mocha

mattkime added a commit that referenced this pull request Jan 3, 2019

Make test runners available from `yarn run test:*` (#26671) (#27969)
* yarn test:mocha yarn test:jest, x-pack too

* remove inline comments, deprecate test:server and replace with test:mocha
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment