search

eclipse-ee4j / mojarra

Mojarra, a Jakarta Faces implementation
Other
158 stars 107 forks source link

Interim commit for #4480 #5319

Closed mnriem closed 8 months ago

BalusC commented 9 months ago

https://github.com/eclipse-ee4j/mojarra/issues/4480

BalusC commented 8 months ago

Looks good!

Only why are unit tests hard removed? Shouldn't these be moved to faces/tck?

The conflicting files section below highlight a painful backwards incompatibility with earlier versions tho. Perhaps best to wait until 4.1 is released before sorting out conflicts and then merging.

mnriem commented 8 months ago

@arjantijms @BalusC They were not conflicting when I submitted the PR. So I will resolve these and ask for a quick review again and then merge it to 5.0 if you are both OK with that?

BalusC commented 8 months ago

They were not conflicting when I submitted the PR

Indeed. These files were changed on target branch after you submitted the PR. It's just that this type of conflict shows that we cannot anymore simply merge 4.x into 5.x for future changes to 4.x.

if you are both OK with that

Can you answer why the unit tests were completely removed? Shouldn't these be moved to faces/tck?

mnriem commented 8 months ago

@BalusC Which is why an umbrella issue is good to have that shows which PRs went where. I also always start with the most recent version first and then execute backports.

mnriem commented 8 months ago

@BalusC @arjantijms I realize this is not the end of splitting the API out. Next is to make sure Glassfish is fine with the split. Which means we need someone to add the new API jar to that project. @arjantijms Is that something you can do?

BalusC commented 8 months ago

How about these deleted tests? Shouldn't we move them to the Faces project?

mnriem commented 8 months ago

@BalusC I have restored the tests, but as they are very tied to the Mojarra implementation I left all but one test in the implementation project. Can we now go ahead and merge this in? Thanks!

BalusC commented 8 months ago

Yes I understood that they've some com.sun.faces deps but all these tests against API classes could simply be moved into the Faces project as part of TCK?

mnriem commented 8 months ago

@BalusC Next step now would be to move the module to the API project :)