@flamisz opened this Pull Request on April 15th 2021 Contributor

Description:

I had a failing test in another PR and it was hard to understand why and where it is happening. Modifying the phpunit.xml can help investigate failing tests easier.

Instead of only dots, we have some information about what's going on, and if a build fails, we can see which one was the last test.

An example:

Api Get Report Metadata (Piwik\Tests\System\ApiGetReportMetadata)
 ✔ Api with data set <a href='/0'>#0</a> [548.94 ms]
 ✔ Api with data set <a href='/1'>#1</a> [26.24 ms]
 ✔ Api with data set <a href='/2'>#2</a> [28.44 ms]
 ✔ Api with data set <a href='/3'>#3</a> [29.22 ms]
 ✔ Api with data set <a href='/4'>#4</a> [7.41 ms]
 ✔ Api with data set <a href='/5'>#5</a> [3.97 ms]

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@sgiehl commented on April 15th 2021 Member

Personally I wouldn't enable that by default in phpunit.xml.dist. If you want that locally, you can simply create a phpunit.xml that contains it. Imho seeing which tests are running is only a useful information while debugging. In all other cases you only need to know which tests are failing - and that is also available without testdox. Having it enabled also makes the output very very long, which also makes it slower to load the travis output in the browser.

@flamisz commented on April 15th 2021 Contributor

Personally I wouldn't enable that by default in phpunit.xml.dist. If you want that locally, you can simply create a phpunit.xml that contains it. Imho seeing which tests are running is only a useful information while debugging. In all other cases you only need to know which tests are failing - and that is also available without testdox. Having it enabled also makes the output very very long, which also makes it slower to load the travis output in the browser.

In my case was different, I wanted to know where it failed on Travis because failed differently, but anytime we can just commit it when needed (probably or hopefully very rare) and when we fixed it, modify it back. This is what I did. I just wanted to know the team thinks about it :)

@tsteur commented on April 15th 2021 Member

Personally finding it quite useful because it'll be there when you need it, and doesn't really cause too much harm when it's not needed. For me it loads as fast, the scrolling to the end takes a second though but that's it. Finding it also quite good to see how long each test took etc. The timings are not ongoing information you need every time though. Other people might not know how and where to set this flag when tests are not executing so it working instantly could be quite good. Especially also for community contributors etc. Luckily, we don't really have much problems anymore with builds timing out otherwise it would have been useful. I can also see though that it does add noise and we could enable it on demand but then ideally there be some output in travis run like this (could even include a link to an FAQ where we explain how to do it)
image
Overall no big preference and not seeing too much harm with adding it but ok either way if we at least add such a message.

@diosmosis any thoughts?

@diosmosis commented on April 15th 2021 Member

Not really, either way is fine for me.

@flamisz commented on April 15th 2021 Contributor

@tsteur I don't think that "trigger another test run by doing...". if you modify the phunit.xml.dist to set the testdox to true, that will trigger the new test run anyway. Or I miss something?

And for me it wasn't an issue either, the result was longer on travis.

@flamisz commented on May 12th 2021 Contributor

@tsteur @sgiehl @diosmosis
Do we want to merge this or I can close it?

@tsteur commented on May 12th 2021 Member

@flamisz @sgiehl @diosmosis I would say let's just merge it and disable it again (and then instead print a message on how to activate this feature to troubleshoot when needed) should we find it not useful. Be good to simply give it a try for a while and if we find it a problem then change. In UI tests we've always been doing this and it never was an issue. @sgiehl @diosmosis @flamisz be important to mention if/when it becomes annoying (eg with heaps of other test failures or so) and then we change it again. Be also useful if it ever helps you to mention eg in slack channel or so "Hey this just helped me save time or find xyz".

@diosmosis commented on May 12th 2021 Member

It makes sense to try it first.

@sgiehl commented on May 17th 2021 Member

Let's simply merge then. We can later remove again if it might get annoying

This Pull Request was closed on May 17th 2021
Powered by GitHub Issue Mirror