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

xml tag order #69

Merged
merged 3 commits into from
Oct 2, 2017
Merged

xml tag order #69

merged 3 commits into from
Oct 2, 2017

Conversation

aerydna
Copy link
Contributor

@aerydna aerydna commented Sep 27, 2017

fixtures and test are now executed following the xml order

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 56.273% when pulling a80cda2 on xmlTagOrder into 6c7763a on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 56.273% when pulling a80cda2 on xmlTagOrder into 6c7763a on master.

@traversaro
Copy link
Member

The coveralls failed report is a bit difficult to analyse (there seem to be something missing , and so it is not possible to visualize the actual for which the coverage changed). For the time being, I would disable it.

@@ -26,12 +26,16 @@ TestSuit::~TestSuit() {
}

void TestSuit::addTest(RTF::Test* test) {
tests.insert(test);
tests.push_back(test);
Copy link
Member

Choose a reason for hiding this comment

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

To maintain the same semantics, we should make sure that we don't add two times the same test pointer in the vector. Without this check, the removeTest would need to be improved.

@traversaro
Copy link
Member

I increased the "Coverage Decrease Threshold for failure" to 3 % , so we can keep the coveralls status comment without spurious failures.

@traversaro
Copy link
Member

@apaikan
Copy link
Contributor

apaikan commented Oct 1, 2017

So far I remember, there was a specific reasons that set has been used over vector in different places.

  • Take care of duplicated tests! set dos not allow by its nature but you should take care of that by yourself if using vector. Duplicated tests are happen very often since users simply copy/past into XML file or when testrunner recursively looking into different folders to load tests and test suites.
  • Cannot we simply use tests.insert(tests.end(), test); ?

@apaikan
Copy link
Contributor

apaikan commented Oct 1, 2017

alternatively we could introduce a new property into our xml tag and propagate it into the Test class to deal with the tests order:

    <test type="dll" param="--from camera_right.ini" priority="1"> CameraTest </test>
    <test type="dll" param="--from camera_left.ini" priority="2"> CameraTest </test> 

Then use native set::sort before lunching the tests?

This would also has distinct advantage for future development when someone want to use different representation/formatting of tests than XML such as 'yaml' or json.

@aerydna
Copy link
Contributor Author

aerydna commented Oct 2, 2017

@apaikan answer to your question in order:

Take care of duplicated tests! set dos not allow by its nature but you should take care of that by yourself if using vector. Duplicated tests are happen very often since users simply copy/past into XML file or when testrunner recursively looking into different folders to load tests and test suites.

  • actually the key for the set is the pointer itself and not some sort of string in the xml.. so it seems adding two times the same test was perfectly legal also with sets instead of vector. also i'm ok for the test being executed two times if there are two identical lines in the xml.

Cannot we simply use tests.insert(tests.end(), test); ?

  • set doesn't guarantee the position of an element (for instance, sets are usually implemented as balanced tree.. where the order is given by the balancing rule). With that overload of insert you can only give a hint.

alternatively we could introduce a new property into our xml tag and propagate it into the Test class to deal with the tests order

this was also one of my idea of improvement initially.. but i think it is more intuitive, at least for the xml case to read the order of the tests and fixtures in a top/down way. Moreover at the moment is the smoothest possible fix

@apaikan
Copy link
Contributor

apaikan commented Oct 2, 2017

great! so we could potentially have dublicated tests. :D
I also noticed that there is not neither overloaded operator ==() in Test class nor checks on the test names to avoid dublicated tests.

then lets keep it as you said untill I recall what was the reason to not make Test class unique or it was simply a bug!
thanks @aerydna !

@aerydna aerydna force-pushed the xmlTagOrder branch 2 times, most recently from 94e2d32 to 9787619 Compare October 2, 2017 13:18
@aerydna aerydna changed the base branch from master to devel October 2, 2017 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants