-
Notifications
You must be signed in to change notification settings - Fork 197
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
Integration test for Maven reactor make behaviours #564
Integration test for Maven reactor make behaviours #564
Conversation
WIP integration test for reactor make options. This may not be a very efficient or robust way to test behaviour. The "resume from" (-rf) option is not yet tested. |
@joeshannon we can always improve extend it and an test not covering all cases is better than no test at all, lets see what the CI build do with it :-) |
@joeshannon many thanks for this useful examples! I have noticed that I need to specify e.g. this runs sucessful
ignoring already build artifacts fails:
|
@laeubi Yes I believe that is correct - if the artifact is available in a repository, that version will be used to build specified reactor projects if it's not otherwise included in the reactor. I've been testing these use cases with a plain maven build with pom dependencies. |
.../src/test/java/org/eclipse/tycho/test/reactor/makeBehaviour/MavenReactorMakeOptionsTest.java
Show resolved
Hide resolved
/rebase |
f721332
to
eaae267
Compare
Hm, isn't this missing the Also, I cannot get things to work as expected. E.g., building
I'd expect only However, the result is this:
Debug log shows:
So far so good. But later:
|
correct, do you like to add one?
The initial project set is that set of project that the default maven would have computed to be build upon your request.
Probably because I do not got it correct how it is supposed to work. As feature 2 requires feature1 I assumed it is an upstream candidate? In any case you can provide the build with |
I cannot modify this PR, @joeshannon could you add one?
But since
Sure, see attachment log.log |
Tycho doesn't care about modules, and as this the parent is "just there" If you do not specify -X its a bit more convient to read, anyways here is the summary:
So we add |
Ah, yes. I think upstream/downstream are switched in the current implementation. -am X / --also-make X == build X and everything X depends on == build X and upstream of X So |
These test the functionality of TychoGraphBuilder (provided by the tycho-build extension) for a simple reactor build.
eaae267
to
0f689a8
Compare
Thank you for the discussion and hints. In the initial revision I had accidentally made a dependency from bundle2 -> bundle1 which confused me for a while. I think the behaviour is as expected now but might need a little more thought to confirm (btw if you would like to see the simple pure maven project I was using to test the options against, I can provide that). I have changed the "child"
I think this is correct since the streams/iterations over The only other thing that concerned me is that these tests do take a while to run (~80s on my machine) due to maven being invoked many times, is this acceptable for the its? Also the failure message strings are potentially brittle and could be broken by string formatting changes - I wasn't really sure of a better way to test this. |
Thanks for looking into this. Actually the integration-tests are the real 'source-of-truth' and are thus of high value for keeping all the stuff working so we generally don't mind the runtime but are working on make them execute more in parallel so don't mind here. So as tests are fine and we have not (yet) any complains I'll merge this, if any issue arise we can extend the test and code in further steps! |
Interesting. Maybe we should have an integration test that tests that over more than one layer? what do you think? |
More tests are always better :-) |
By the way, there is also some room for improvements if only dependent projects are requested one do only need to resolve the dependencies for those (currently all projects are calculated for dependencies) if we do not need to add dependent-of-dependecies. |
Actually it should give all dependencies but as mentioned earlier, this is currently a bit experimental as we do allow incomplete resolving and e.g. do not take the target platform into account. But given that the reactor project set is rather well-defined also without the target (what should be the case in most scenarios) I don't see a problem. I just wonder for the "build what depends on" case, e.g. I have feature1 depends on feature2, now I request feature2+everything that depends on it, I need to add everything feature1 depends to get a full set as feature2 will only add feature1 thats why I added all transistive dependencies of all extra selected projects. |
Correct. In the current state the following currently fails using this set of integration test projects:
because it picks up I propose to add the flag back. Otherwise the |
@sratz can you add a bug (targeted at 2.7.0) and extend the integration test for this purpose? |
No description provided.