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

Add support for ember test run configuration #166

Merged
merged 1 commit into from
Jul 10, 2018

Conversation

makepanic
Copy link
Contributor

@makepanic makepanic commented Oct 24, 2017

This PR adds a run configuration that attaches the generated console to intellij com.intellij.execution.testframework.sm.SMTestRunnerConnectionUtil which generates the integrated test ui:

20171021-121058

There isn't any conversion going on because it only pipes testem teamcity output to the test runner.
This means we don't get the most user friendly output right now but once testem/testem#1188 and testem/testem#1187 are resolved, it should display durations and group the tests as seen above.

This PR closes #125

I've added a small paragraph about the test run configuration, without a screenshot, to features.md.

@makepanic makepanic force-pushed the run-configuration-ember-t branch 2 times, most recently from 8af0a8f to 58a1794 Compare October 24, 2017 12:08
@Turbo87
Copy link
Owner

Turbo87 commented Oct 26, 2017

Regarding the dialog I have a few suggestions:

  • "Test port" is an advanced setting IMHO
  • "Environment" too because it is very uncommon to change it (also the default in this case should be "test", not "development")
  • "Filter" should be a non-advanced setting (I use that a lot)
  • "Module" probably too (I personally don't use it but know a few people that do)
  • Maybe we need something like the radio buttons when you setup e.g. a "Jest" test (All tests, Module with input field, Filter with input field)
  • The "Launch" option is not represented. Maybe radiobuttons would make sense here too (Default Browsers, Specific Browsers with input field, No Browsers --no-launch)

@Turbo87
Copy link
Owner

Turbo87 commented Oct 26, 2017

bildschirmfoto 2017-10-26 um 11 15 25

looks like browser grouping didn't work for me 🤔

is it possible to hide the 0ms labels until we have knowledge of the actual runtime of the tests?

@Turbo87
Copy link
Owner

Turbo87 commented Oct 26, 2017

and do you think it would be possible to implement the "jump to test code" behavior that is available for other test frameworks? might be something for a future PR

@makepanic
Copy link
Contributor Author

makepanic commented Oct 26, 2017

looks like browser grouping didn't work for me 🤔

Grouping only works once the teamcity reporter in testem is updated. see testem/testem#1187

is it possible to hide the 0ms labels until we have knowledge of the actual runtime of the tests?

I don't think so. Intellij automatically shows the duration even if it's not provided by the output.
Testem reports singular test run durations once testem/testem#1188 is merged

and do you think it would be possible to implement the "jump to test code" behavior that is available for other test frameworks? might be something for a future PR

It's possible but requires some additional work. I agree that we should postpone this feature for a future PR.

Regarding the form changes, I'll fix that as soon as i have time for it. (Prob later this day)

@makepanic makepanic force-pushed the run-configuration-ember-t branch from 58a1794 to 9c89c8a Compare October 30, 2017 10:28
@makepanic
Copy link
Contributor Author

I've modified the form based on the issues mentioned above.

20171030-111019

Sadly the form gets pretty tall like that.

Maybe we need something like the radio buttons when you setup e.g. a "Jest" test (All tests, Module with input field, Filter with input field)

Could you elaborate more on how we can design this in a testem/ember context?

@Turbo87
Copy link
Owner

Turbo87 commented Nov 6, 2017

Sadly the form gets pretty tall like that.

The "Launch" option is not represented. Maybe radiobuttons would make sense here too (Default Browsers, Specific Browsers with input field, No Browsers --no-launch)

We don't need to show the "Launchers" list if the radio buttons (mentioned above) are set to "default launchers" (which should obviously be the default) or "no launchers", so in those cases the form is less tall.

Could you elaborate more on how we can design this in a testem/ember context?

similar to:

bildschirmfoto 2017-11-06 um 14 21 42

  • All tests: don't show any input fields and use no filters
  • Module: show "Module" input field and use -m flag
  • Filter: show "Filter" input field and use -f flag

@makepanic makepanic force-pushed the run-configuration-ember-t branch 3 times, most recently from 1f5edbb to 67f66c8 Compare November 17, 2017 12:35
@makepanic
Copy link
Contributor Author

I've made some changes to include buttongroups for filters and launchers.

20171117-121108

I think the No Browser launcher won't work because it's not really possible to pass --launch=

@Turbo87
Copy link
Owner

Turbo87 commented Nov 17, 2017

can you move the separator line of the browsers list above the radio button group?

I think the No Browser launcher won't work because it's not really possible to pass --launch=

you can use --no-launch in that case

@makepanic
Copy link
Contributor Author

we could also remove the line all together
20171117-141121

@Turbo87
Copy link
Owner

Turbo87 commented Nov 17, 2017

yeah, but I think it could be useful to group the optiongroups and the browserlist together

@makepanic makepanic force-pushed the run-configuration-ember-t branch from 67f66c8 to 9914884 Compare November 17, 2017 14:16
@makepanic
Copy link
Contributor Author

I've found out how jetbrains adds annotated borders:

20171117-151120

Using --no-launch gives the same testem error: Launcher false not found. Not installed?

@Turbo87
Copy link
Owner

Turbo87 commented Nov 17, 2017

Using --no-launch gives the same testem error: Launcher false not found. Not installed?

that is very strange. I always use ember test with --no-launch through a shell alias 🤔

@makepanic
Copy link
Contributor Author

If i do it manually i get:

> ember t --no-launch
...
<some log>
...

  F

  1 tests complete (711 ms)

  1) [null] Error
     Launcher false not found. Not installed?

@Turbo87
Copy link
Owner

Turbo87 commented Nov 17, 2017

ah, maybe that option only works if used with --server 🤔

@makepanic
Copy link
Contributor Author

We don't support the --server flag currently. Should i remove the No Browser option?

@Turbo87
Copy link
Owner

Turbo87 commented Nov 17, 2017

We don't support the --server flag currently. Should i remove the No Browser option?

yeah, I guess that makes sense. sorry for the confusion :(

@makepanic makepanic force-pushed the run-configuration-ember-t branch from 9914884 to 23496f0 Compare November 17, 2017 15:36
@makepanic
Copy link
Contributor Author

Ok, I've remove the No Browser option

@dwickern
Copy link
Collaborator

@makepanic The run configuration seems to assume that the ember module is the root of the project. If it's in a subdirectory you get an error:

Error running 'ember test': Cannot run program "/Users/dwickern/code/my-project/node_modules/.bin/ember"

@makepanic
Copy link
Contributor Author

@dwickern thanks for the feedback, can you check if this error also appears when generating an ember file through the plugin?

@makepanic makepanic force-pushed the run-configuration-ember-t branch from 9d3b606 to f695123 Compare December 1, 2017 20:20
@Turbo87 Turbo87 force-pushed the run-configuration-ember-t branch from 567ba4e to f860ac8 Compare June 7, 2018 11:09
@Turbo87
Copy link
Owner

Turbo87 commented Jun 7, 2018

@makepanic I've just tried this out. A few things that I noticed:

  • In the "Launchers" section nothing is selected by default. I guess "Default Browsers" should be selected by default.

  • There is no console output visible, so when ember test fails it should tells you that it quit unexpectedly, but not why. Is there anything we can do about that?

@Turbo87
Copy link
Owner

Turbo87 commented Jun 7, 2018

also, I've rebased this PR on the current master :)

@Turbo87 Turbo87 mentioned this pull request Jun 7, 2018
@Turbo87 Turbo87 changed the title feat(Configuration): add support for ember test run configuration Ddd support for ember test run configuration Jun 7, 2018
@Turbo87 Turbo87 changed the title Ddd support for ember test run configuration Add support for ember test run configuration Jun 7, 2018
@Turbo87 Turbo87 force-pushed the run-configuration-ember-t branch from f860ac8 to c0c85ba Compare June 7, 2018 13:07
@joegaudet
Copy link

So, stoked for this by the way!

@makepanic makepanic force-pushed the run-configuration-ember-t branch from c0c85ba to 661e695 Compare June 10, 2018 19:22
@makepanic
Copy link
Contributor Author

makepanic commented Jun 10, 2018

Thanks for rebasing.

I've updated the PR so it should fix:

  • In the "Launchers" section nothing is selected by default. I guess "Default Browsers" should be selected by default.

I think the missing error message may be a bug in the teamcity reporter.
Intellij gets the following stdout:

##teamcity[testFailed name='Firefox 60.0 - Unit || Model || foo: … <em>tesst</em> …' message='' details='@http://localhost:7357/assets/tests.js:869:7|nrunTest@http://localhost:7357/assets/test-support.js:14734:16|nrun@http://localhost:7357/assets/test-support.js:14720:6|nrunTest/<@http://localhost:7357/assets/test-support.js:14932:7|nadvance@http://localhost:7357/assets/test-support.js:14372:6|nbegin@http://localhost:7357/assets/test-support.js:16321:4|ninternalStart/config.timeout<@http://localhost:7357/assets/test-support.js:15319:6|n'

compared to tap output:

not ok 273 Firefox 60.0 - Unit | Model | foo: … <em>tesst</em> …
    ---
        actual: >
            … &lt;em&gt;tesst&lt;/em&gt; …
        expected: >
            … &lt;em&gt;test&lt;/em&gt; …
        stack: >
            @http://localhost:7357/assets/tests.js:869:7
            runTest@http://localhost:7357/assets/test-support.js:14734:16
            run@http://localhost:7357/assets/test-support.js:14720:6
            runTest/<@http://localhost:7357/assets/test-support.js:14932:7
            advance@http://localhost:7357/assets/test-support.js:14372:6
            begin@http://localhost:7357/assets/test-support.js:16321:4
            internalStart/config.timeout<@http://localhost:7357/assets/test-support.js:15319:6
            
        Log: |
    ...

I'll investigate and maybe create an upstream issue.

p.s. i think squash rebasing my changes removed your master rebase information. Meaning the PR only includes changes from me now 🤔

edit: I've created testem/testem#1269 for comparison error results

@Turbo87
Copy link
Owner

Turbo87 commented Jun 13, 2018

I think the missing error message may be a bug in the teamcity reporter.
Intellij gets the following stdout:

This is not about errors in the tests, it's about errors when starting up ember test like for example a dependency not being installed.

@makepanic makepanic force-pushed the run-configuration-ember-t branch from 661e695 to ce2eff3 Compare June 16, 2018 18:12
@makepanic
Copy link
Contributor Author

I've pushed a change that removed stdout filtering.

That way the teamcity report lines get processed by intellij but the other output isn't lost.

20180616-200643
20180616-200654

@Turbo87 Turbo87 force-pushed the run-configuration-ember-t branch from ce2eff3 to 5b7a635 Compare July 10, 2018 10:38
@Turbo87 Turbo87 force-pushed the run-configuration-ember-t branch from 5b7a635 to 73b9e61 Compare July 10, 2018 11:03
@Turbo87
Copy link
Owner

Turbo87 commented Jul 10, 2018

works perfectly now, great work @makepanic!

@Turbo87 Turbo87 merged commit 22fd10c into Turbo87:master Jul 10, 2018
@makepanic makepanic deleted the run-configuration-ember-t branch July 13, 2018 19:08
@joegaudet
Copy link

@makepanic are there any plans to support server mode, the rebuild every test run makes this quite slow.

@makepanic
Copy link
Contributor Author

makepanic commented Feb 28, 2019

@makepanic are there any plans to support server mode, the rebuild every test run makes this quite slow.

You mean ember test --server?

The issue with that is that it spawns a testem instance in development mode with a TUI that we can't hook into.
I guess if we find a way to start ember test --server in a "silent" mode, which emits test reporter output, while not blocking the process with TUI, we can improve this.

Maybe @johanneswuerbach knows if something like that is possible.

I agree with our current setup, it's kinda useless as it will always cause a full build.

@johanneswuerbach
Copy link

Sorry I step out of frontend development some time ago. Technically it’s possible to run testem dev mode with different reporters (not just the TUI), but I don’t think this is exposed as a flag :-(

@makepanic
Copy link
Contributor Author

No worries :) Thanks for all the great work you've done with testem.

I'll try to see if I can get testem dev work to run with the teamcity reporter in ember-cli.

@joegaudet
Copy link

@makepanic thanks for all your hard work on this.

Looked through the docs and don't see anything obvious about getting different reporters while in dev mode.

@joegaudet
Copy link

@makepanic that being said it does appear to be supported in the config file.

testem/testem#1289

@joegaudet
Copy link

@makepanic
Copy link
Contributor Author

So i've hacked around in testem source and overwrote the forced dev reporter in dev with the teamcity one.

https://github.com/testem/testem/blob/master/lib/config.js#L45

Turns out if we then launch the test run configuration with the server flag, it works out of the box.

A potential issue is that intellij adds all tests up, even if rerun, meaning the longer you work the more reported tests you get.
We might have to investigate clearing the output in case of a rerun.

If that's something we want to do, i guess the plan would be:

  • add config to testem that allows to customize the dev reporter
  • add optional? test runner config to launch as server
  • investigate if we can clear the test output

@joegaudet @Turbo87 do you think it's something useful? If so I can start doing above

@joegaudet
Copy link

@makepanic absolutely, this feature would make ember test writing significantly better.

Let me know if there is anything I can do to help.

Some additional thoughts.

  • Would this test runner be able to connect to the IntelliJ debugger in any way (would be incredible)
  • Any ability to launch a single test assertion using the right click functionality?

@makepanic
Copy link
Contributor Author

* Would this test runner be able to connect to the IntelliJ debugger in any way (would be incredible)

I haven't looked into this. Maybe #109 could be investigated.

* Any ability to launch a single test assertion using the right click functionality?

I've previously worked on this in 2017 but never finished it. I think we could add this later.
Basically the RunLineMarkerContributor in intellij creates a run configuration. This means because we already use test run configurations we would simply need to create the correct one (with prefilled filters).

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

Successfully merging this pull request may close these issues.

5 participants