Skip to content
This repository has been archived by the owner on Aug 8, 2020. It is now read-only.

Support running all tests in project #18

Merged
merged 38 commits into from
Jul 7, 2016

Conversation

jacobmendoza
Copy link
Collaborator

@jacobmendoza jacobmendoza commented May 29, 2016

Modifications to support running all tests in project. Looking for some feedback to see if everything looks in the right direction and some implied assumptions are fine.

  • The order of the files appears in the same order returned by the TAP reporter.
  • I haven't removed TerminalCommandExecutor yet but this class is now using BufferedProcess. BufferedProcess produces side effects as it already spawns the process and could make testing harder as it is hard to mock. I'm trying to figure out this before completely removing the class. For now, acts as a wrapper (related: Replace TerminalCommandExecutor with BufferedNodeProcess? #16).
  • As suggested, the package will execute all tests with ctrl-alt-a and the current file with ctrl-alt-shift-a.
  • An editor screen must be opened to deduce the project.
  • This extension needs AVA 0.15 to work properly (avajs/ava@3c4babd). Previous versions will display the ANSI scape codes. I think I should be removing them also to support previous versions, not done yet.

This feature was already started, but IMO next step should be migrate the test suite to AVA.

screen shot 2016-05-29 at 18 01 04

Related: #13

"ctrl-alt-a": "ava:toggle"
"ctrl-alt-shift-a": "ava:run",
"ctrl-alt-a": "ava:run-all",
"ctrl-alt-t": "ava:toggle"
Copy link
Member

Choose a reason for hiding this comment

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

I think we should drop this shortcut. It's already covered by running. We can rather add support for pressing Esc when the pane is active to close it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Researching about this. Having some problems to detect when the pane is active. Also, the part of the application that is usually active when running tests is the editor.

I'm trying to figure this out.

@jacobmendoza
Copy link
Collaborator Author

Added an initial but hopefully reasonable implementation of the ESCAPE key for closing the pane. Based on: https://github.com/atom/find-and-replace/blob/master/lib/find.coffee#L78.

For our case:

  • Used core:cancel. Not used core:close as the panel may not be related specifically to the current file and you want to keep it visible.
  • You need to have selected the main editor, which will be active when running tests.
  • Events triggered from the mini-editor (that is, other components) will be skipped.
  • Toggle command and keyboard shortcut removed.

@@ -19,5 +19,5 @@
<div class="sk-child sk-bounce3"></div>
</div>
</div>
<div id="file-header"></div>
<div class="tests-container"></div>
<div id='tests-groups-container' class='tests-groups-container'>
Copy link
Member

Choose a reason for hiding this comment

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

Double-quotes for HTML attributes.

@jacobmendoza
Copy link
Collaborator Author

@sindresorhus, does this look mature enough to be merged?

group: '',
currentExecution: context
};
module.exports.getAssertResult = function (assert, context) {
Copy link
Member

Choose a reason for hiding this comment

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

Just export it directly:

module.exports = (assert, context) => {

Note arrow function too.

And just rename the file to get-assert-result.js.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, agreed!. Thank you so much @sindresorhus

@sindresorhus
Copy link
Member

Only one thing and it's ready: #18 (comment)

@jacobmendoza
Copy link
Collaborator Author

@sindresorhus, as always, very grateful for all the feedback. Really appreciated.

@jacobmendoza
Copy link
Collaborator Author

jacobmendoza commented Jul 3, 2016

Found and edge case yesterday. When the user selected 'run all the files' but only one test was in the folder, the package was unable to render the results.

On my side, ready to merge now @sindresorhus .

@sindresorhus sindresorhus merged commit f0dff00 into master Jul 7, 2016
@sindresorhus sindresorhus deleted the support-running-all-tests-in-project branch July 7, 2016 09:28
@sindresorhus
Copy link
Member

Excellent. Works great! Thanks for your hard work on this :)

sindresorhus added a commit that referenced this pull request Jul 7, 2016
@jacobmendoza
Copy link
Collaborator Author

Thank you @sindresorhus for all your feedback and support. Learning lots of things!.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants