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

Run and show test results in the editor #8

Merged
merged 47 commits into from
May 14, 2016
Merged

Conversation

jacobmendoza
Copy link
Collaborator

Overview: Prototype for integrating a runner of AVA tests in Atom. Not ready to merge, just requesting feedback (#3).

Steps: Link the package. At this stage, there is no menu integration (yet), so pressing ctrl-alt-a is needed to activate the package (right pane will appear). If a test file is active, pressing ctrl-alt-x will trigger the execution of the tests. Every time that one assert is processed, a new line will appear in the UI (the feedback during execution needs to be improved).

About the code: It's just a prototype, so probably there are a lot of things to discuss. Intentionally, there is no effort in this PR for formatted and nice UI. I'm using this parser for the TAP output, and seems to work very well. Anyway, the idea is to abstract the interaction with it, just in case we could need to replace it (with this code, this is only partially achieved).

Part of the pageres suite

@sindresorhus
Copy link
Member

Good start! I know we didn't discuss this upfront, but the plugin should use ES2015 (ES6), not CoffeeScript. Nothing against CoffeeScript, but ES2015 is more future friendly and better for attracting potential contributors.

At this stage, there is no menu integration (yet)

What kind of menus were you thinking? I think it's more important to implement command palette commands than menus.

@@ -0,0 +1,3 @@
'atom-workspace':
'ctrl-alt-a': 'ava:toggle'
'ctrl-alt-x': 'ava:run'
Copy link
Member

Choose a reason for hiding this comment

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

Is there really any point in having two separate shortcuts? I would think it's enough with just Ctrl+Alt+A which toggles and runs tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no big reason for having two today. I used two because they allowed me to easily test in isolation the two steps. I'll compact them in one.

If in the future we have a parsing of the test file (something that I'd like to discuss after having a nice stable first version) phase we'll need two. No need to worry about this now.

@jacobmendoza
Copy link
Collaborator Author

I wasn't aware of this but these are actually good news. No doubt this is more future friendly and has more potential. I'll start doing some testing.

The comment about the menu did not have a big foundation, I was just trying to say that there is no visual entry point in the UI. Probably we don't need it.

@jacobmendoza
Copy link
Collaborator Author

Thanks for all the feedback :). I'll try to push a new version with these issues addressed asap.

@jacobmendoza
Copy link
Collaborator Author

I have addressed some of the issues previously marked:

  • The files are now ES6.
  • Dependencies fixed.
  • I'm still not using the programmatic API from AVA. Pending.
  • Ctrl+A toggles the package and runs the tests.
  • I haven't done anything with the UI. At this stage is only showing the tests. There is no method to show errors.

I have tried to implement a non very successful cancellation mechanism. I'm not sure if it's the best approach, but the idea is stop the process if the user closes the package or if the test execution process is launched several times. It'd great have some feedback about the appropriate way to do it with the programmatic API.

Is it looking any better?. Thank you!.

@@ -0,0 +1,34 @@
'use babel';
Copy link
Member

Choose a reason for hiding this comment

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

Use /** @babel */ instead.

@jacobmendoza
Copy link
Collaborator Author

jacobmendoza commented May 2, 2016

@sindresorhus, @jamestalmage, if you have time it would be great if you can give it a go and let me know what you think:

Video
(When the video was recorded both columns had 'passed' as name. It has been fixed)
screen shot 2016-05-02 at 17 39 33

Some minor changes in the UI to prepare the plugin for execute several files. The idea is that we will have several blocks as the one featured in the video (one per file).

My initial understanding is that some of the features that I feel like missing (block decoration and multiple files) will require some minor modifications in the TAP reporter of AVA, that is why I would like to know if the features that we have make sense as an MVP before going for that second phase.

Thanks!

@sindresorhus
Copy link
Member

@jacobmendoza This looks really great! 🙌 More than good enough for a MVP.

Some minor nitpick. When using the Atom Dark theme some things could be tweaked:

screen shot 2016-05-04 at 22 03 11

Maybe use rgba() with lowered opacity on the horizontal line and the test.js background. Or maybe there's a built-in theme background variable we could use.

And instead of the current x for closing the sidebar.

And I get this when trying to run a test file from the got project. Maybe because it's in a nested folder? I'm running the command directly from the file.

screen shot 2016-05-04 at 22 10 59

@jacobmendoza
Copy link
Collaborator Author

Hey @sindresorhus, thanks for spending some time with this!.

  1. I had in mind the style for the dark theme and I forgot in the last minute. Yeah, we should have some variables in place that should help us. I'll take a look.

  2. I've changed the close icon already. Will push the commit with all the other changes.

  3. That last error that you have there is strange!. I have cloned the project and tried to reproduce. I have opened atom in the project folder, and run the file (gzip.js) with the command and with the keyboard shortcut. Worked for me with both of them, also for the other files of the project. Is it happening just for that one?.

@jamestalmage
Copy link

Very cool.

Only thing I've noticed so far is that skipped tests show as passed.

@jfmengels
Copy link

Only thing I've noticed so far is that skipped tests show as passed.

And todo tests are shown as failed.

Looks like a great start! Great job @jacobmendoza!

@jacobmendoza
Copy link
Collaborator Author

Hey @jamestalmage, @jfmengels!.

Thanks for trying out the package! :). Yeah, that's important, something funny with the parsing process I'd say. Hopefully it'll be fixed very soon.

@jamestalmage
Copy link

@jacobmendoza
If it's a parsing problem, that could very well be AVA's fault. Our TAP output isn't super sophisticated.

https://github.com/jamestalmage/tap-emitter aims to fix that, but I've kind of let that languish. I would love extra help on that. It would require fixing avajs/tap-emitter#2 and integrating into AVA. No pressure, but it might make your life easier to improve the TAP output.

@jacobmendoza
Copy link
Collaborator Author

@jamestalmage, not sure yet if it's the output that AVA generates or actually the parsing process. I think I'll be able to confirm today.

Anyway, yeah, I would be more than happy to help. This is pretty far from my usual technology stack, so I may need some help for start. Actually, regarding the TAP output, there are a few ideas that I wanted to check with you, but I was trying to get something basic working here before.

I'll keep you posted!

@jacobmendoza
Copy link
Collaborator Author

jacobmendoza commented May 5, 2016

@jamestalmage, @jfmengels, thanks for your comments yesterday. They were really helpful.

  1. About running tests that were marked as skip/todo, should be working now. AVA is doing just fine, and literally, it wasn't implemented (6d6449a - edit: just noticed a style defect there). For now, they just won't appear in the summary and statistics and they will appear in the list in a different colour. If you have an opinion about the presence/shape that they should have in the UI let me know!. Used the most basic approach for now.

  2. @jamestalmage, again, there are more things that I'd like to get from the TAP reporter, but my lack of knowledge is so big that I don't even know if my ideas make sense or what would be the best approach to implement them. I'll contact you after putting this in a reasonable state for a first phase.

  3. The plugin should be looking significantly better in different themes. I've tried with 6 and at least the result is reasonable. I'm using some pre-defined variables of the themes (7d3aa5e).

  4. @sindresorhus, I've just realised. In the package I'm making reference to the AVA logo directly in the AVA repo. If the network fails or is slow, it will look pretty funny. Can I copy it to this package?. The only problem that I see is that we will have two places to maintain the logo if it changes, but even with that, I guess that it should be a local asset of the plugin.

Also, fixed a crash if the user tries to run tests in a window with no active editor.

@sindresorhus
Copy link
Member

Can I copy it to this package?

Yes, should be in the package.

@jacobmendoza
Copy link
Collaborator Author

@sindresorhus, did you take a look at the error that you were having while trying to test got?. Can you still reproduce it?.

I was wondering if, assuming that the defect is no longer present, we could start thinking about merging with the main branch. This PR is starting to get big, and I would like to make a difference between the first basic features and the followings to come.

Thank you!

@sindresorhus sindresorhus changed the title Work in progress - Run and show test results directly in the editor Run and show test results directly in the editor May 14, 2016
@sindresorhus sindresorhus changed the title Run and show test results directly in the editor Run and show test results in the editor May 14, 2016
@sindresorhus sindresorhus merged commit 8e79809 into master May 14, 2016
@sindresorhus sindresorhus deleted the runner-start branch May 14, 2016 13:39
@sindresorhus
Copy link
Member

@jacobmendoza Yes, works great now! This is really awesome. We really appreciate all the hard work you put into this.

superhighfive

@sindresorhus
Copy link
Member

I'll do a new release and tweet about it next week ;)

@jfmengels
Copy link

fmkp70c7IIb9C

@jacobmendoza
Copy link
Collaborator Author

@sindresorhus, @jfmengels,

Thanks a million for your kind words, really ✋✋✋ . I have enjoyed it a lot and it has been great to have your feedback during the process!.

Planning to comment soon #13 to see what you think.

Cheers guys!

@jacobmendoza jacobmendoza mentioned this pull request May 17, 2016
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.

4 participants