fix(teamcity_reporter): nest tests by prefix #1262
Conversation
|
@Turbo87 this appears related to: Turbo87/intellij-emberjs#166 mind confirming this is good? |
|
To confirm that the reporter changes still work, I've loaded it up in ember-intellij with the patched test run configuration:
|
|
@makepanic awesome. That looks great. Once I get @Turbo87's +1, i'll merge and ship. :) |
| assert.match(output, /##teamcity\[testFailed name='phantomjs - it handles undefined errors' message='' details='']/); | ||
| assert.match(output, /##teamcity\[testFinished name='phantomjs - it handles undefined errors' duration='42']/); | ||
| assert.match(output, /##teamcity\[testSuiteStarted name='phantomjs']/); | ||
| assert.match(output, /##teamcity\[testSuiteFinished name='phantomjs']/); |
There was a problem hiding this comment.
I understand that this is currently not testing the order in which these messages are reported, but I guess that might be a good idea to make sure that testSuiteFinished is only printed after all the tests have finished 🤔
There was a problem hiding this comment.
I think what you're asking about is tested in the next test.
The change to this test was only made to get the test to succeed.
| if (this.prefixedResults.hasOwnProperty(prefix)) { | ||
| this.out.write('##teamcity[testSuiteStarted name=\''+prefix+'\']\n'); | ||
| this.prefixedResults[prefix].forEach(this._display.bind(this)); | ||
| this.out.write('##teamcity[testSuiteFinished name=\''+prefix+'\']\n'); |
There was a problem hiding this comment.
is it intended that this._duration() was dropped from the testSuiteFinished event?
There was a problem hiding this comment.
I think previously it was an error as the jetbrains docs mention duration only for Test start/stop messages
|
@makepanic if I understand this patch correctly then previously it would write out test results as they are reported. with this change it would only write out the results at the end of the whole test suite. does that have any negative implications for tools that want to report the test results "live"? |
Right that could be an issue. |
That seems a tad unfortunate for those with long/large test suites. Maybe this should be opt-in? |
|
I guess one could add a new reporter parallel to the old teamcity reporter. Dunno if it's a good idea to ship it in testem or as an extra package. |
|
@makepanic can you check if IntelliJ merges test suites with the same name? then we could open a new test suite per test, and let IntelliJ worry about merging them up so that tests are grouped per suite 🤔 |
Great idea. I guess the teamcity format requires input to be xml'ish. Dunno if the additional overhead of maintaining a new test reporter, additional configuration or introducing a breaking change is worth it tbh. |
|
hmm okay. guess then there is not much we can do about this unfortunately. I'm fine either way, the implementation looks good to me. @makepanic I guess it's up to you. If you think this is the right way forward than I'm 👍on merging this. |
|
I think it's better not to break clients as the change is only cosmetic. As we can't really do it in a backwards compatible way, I'm ok with closing the PR + the issue. |
|
@makepanic in theory, we could make this opt-in? (if there is concern about backwards compat) |
yes that should be possible |
|
@makepanic with opt-in, i see not reason not to merge + release immediately once complete. Just be sure to add the appropriate docs :) |
fixes #1187