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

Interim commit for #4480 #5319

Merged
merged 2 commits into from
Nov 6, 2023
Merged

Interim commit for #4480 #5319

merged 2 commits into from
Nov 6, 2023

Conversation

mnriem
Copy link
Contributor

@mnriem mnriem commented Sep 24, 2023

No description provided.

@BalusC
Copy link
Contributor

BalusC commented Sep 30, 2023

#4480

@BalusC
Copy link
Contributor

BalusC commented Oct 14, 2023

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
Copy link
Contributor Author

mnriem commented Oct 15, 2023

@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
Copy link
Contributor

BalusC commented Oct 16, 2023

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
Copy link
Contributor Author

mnriem commented Oct 16, 2023

@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 mnriem requested review from arjantijms and BalusC November 1, 2023 21:49
@mnriem
Copy link
Contributor Author

mnriem commented Nov 1, 2023

@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
Copy link
Contributor

BalusC commented Nov 4, 2023

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

@mnriem
Copy link
Contributor Author

mnriem commented Nov 5, 2023

@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
Copy link
Contributor

BalusC commented Nov 5, 2023

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?

Copy link
Contributor

@BalusC BalusC left a comment

Choose a reason for hiding this comment

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

Approved nonetheless. We need to move forward.

@mnriem mnriem merged commit 3292dc0 into eclipse-ee4j:master Nov 6, 2023
1 check passed
@mnriem mnriem deleted the issue-4480 branch November 6, 2023 13:40
@mnriem
Copy link
Contributor Author

mnriem commented Nov 6, 2023

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

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.

2 participants