Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Incorrent xml output if I use the dataProvider in tests #185

Merged
merged 4 commits into from
Nov 17, 2015

Conversation

dhopkalo
Copy link

Incorrent xml output if I use the dataProvider in tests #19

@dhopkalo dhopkalo changed the title Incorrent xml output if I use the dataProvider in tests Incorrent xml output if I use the dataProvider in tests #19 Nov 12, 2015
@dhopkalo dhopkalo changed the title Incorrent xml output if I use the dataProvider in tests #19 Incorrent xml output if I use the dataProvider in tests Nov 12, 2015
@julianseeger
Copy link
Contributor

Thanks for the patch.
Could you please add a small test for this?

@dhopkalo
Copy link
Author

I think i will if I will not forget to do it :)
I will add some fixture junit xml for dataProvider and make small test for that.

- add testExtendEmptyCasesFromSuites
- add $nestedSuite to extendEmptyCasesFromSuites
@dhopkalo
Copy link
Author

Pls review I did small test for this.
thx.
@julianseeger

@julianseeger
Copy link
Contributor

Thx. As the fix is referencing #19 which is pretty old, I would merge this PR.

But if you have some time left @drefixs there are some issues that could be fixed before the branch is merged:

  1. even though the log makes much more sense than before (no empty names etc.), it still differs a lot from the phpunit log (compared to phpunit 5.0.9). I don't know how close we need to get, but at least a level of "testsuite" is missing
  2. the tests executes every new line but it doesn't logically cover it
    The lines 94 and 95 contain a lot of logic but the tests only checks for not-emptyness. Accordingly this would make the tests pass:
$class = ".";
$file = ".";

So there is a test missing that checks if the values of $class and $file make sense.
And line 102 if (empty($case->file)) { can be removed without failing the test.

Feel free to fix these issues. ;) It could be merged without further work, but #19 would be left open.

@dhopkalo
Copy link
Author

Thx, for review @julianseeger

  1. According to https://raw.githubusercontent.com/windyroad/JUnit-Schema/master/JUnit.xsd, PHPUnit performs invalid logs. Testsuite mustn't contain itself. There is no point in making log closer to PHPUnit format because it is invalid. The main target of this fix is to solve a critical format requirement to make this log readable for report systems. There isn't enough completeness of data to make this absolutely valid to JUnit.xsd.
  2. Thanks, these are right remarks, and I will make more complex unit tests.

Thanks.

- made extendEmptyCasesFromSuites simple
@dhopkalo
Copy link
Author

I decided to do logic more simple because it's bad idea to try get something from nested testsuite it doesn't aggregate any data from junit.xml log. I think to fix this problem directly there is 2-way. Wait for PHPUnit fix they JUnit log According to JUnit.xsd or rewrite paratest Reader and Writer According to JUnit.xsd and add some adapter to Reader for invalid PHPUnit JUnit logs.

  1. Done pls review :).

@julianseeger
Copy link
Contributor

Looks good. I'll check this tomorrow.

@julianseeger
Copy link
Contributor

Alright, thx for your additional fixes. (2) is fine now and with point (1) you are probably right. There is no hint anywhere that nested testcases are a valid occurence, not even in the phpunit docs as some kind of special case so I would also consider that a bug in phpunit. No point in adding buggy behavior just for compliance.

julianseeger added a commit that referenced this pull request Nov 17, 2015
Incorrent xml output if I use the dataProvider in tests
@julianseeger julianseeger merged commit 834e76b into paratestphp:master Nov 17, 2015
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants