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

Extract parts of XpectRunner singleton #344

Merged

Conversation

trancexpress
Copy link
Contributor

This change extracts parts of the singleton XpectRunner to another class, so that other implementations can set the values of the singleton.

This change is required, in order to provide alternatives of XpectRunner, such as a JUnit 5 alternative, without depending on XpectRunner.

Preparation for: #262

This change extracts parts of the singleton XpectRunner
to another class, so that other implementations can set the values of the singleton.

This change is required, in order to provide alternatives of XpectRunner,
such as a JUnit 5 alternative, without depending on XpectRunner.

Preparation for: eclipse#262
@trancexpress
Copy link
Contributor Author

This is a subset of #343, the minimal set of changes we need to port our Xpect tests to JUnit 5. If the actual PR causes too much debate / overhead or there is no one to review such big changes, this smaller set of changes would also be enough for us.

@trancexpress
Copy link
Contributor Author

trancexpress commented May 26, 2024

@tjeske , could you take a look here? Its a small change, but needed.

Or maybe you can take a look @cdietrich ?

@iloveeclipse
Copy link
Contributor

@cdietrich or @meysholdt : could you please check?

@cdietrich
Copy link
Member

cdietrich commented Jun 5, 2024

@iloveeclipse my capacity this month is non existing. is tobias no longer with you?

@iloveeclipse
Copy link
Contributor

@cdietrich : no idea. @tjeske : are you with us ? :-)

@tjeske
Copy link
Contributor

tjeske commented Jun 6, 2024

LGTM as far as I can tell. Honestly I am not familiar with the initial architecture idea (probably no one is anymore) but it shouldn't make things worse. If this PR solves your problem I am fine with it.

@cdietrich
Copy link
Member

@trancexpress would you be willing to become a contributor?

@iloveeclipse
Copy link
Contributor

@trancexpress would you be willing to become a contributor?

Yes he will :-)

@cdietrich
Copy link
Member

i cannot judge on the thread safty of the new singleton. are we fine with it?

@iloveeclipse
Copy link
Contributor

i cannot judge on the thread safty of the new singleton. are we fine with it?

Do you mean XpectTestGlobalState? It is not thread safe. Are tests run in parallel? If so, singleton doesn't work per definition.

@trancexpress
Copy link
Contributor Author

i cannot judge on the thread safty of the new singleton. are we fine with it?

Its as unsafe as XpectRunner is.

@cdietrich cdietrich merged commit 437ea03 into eclipse:master Jun 6, 2024
2 checks passed
@cdietrich
Copy link
Member

thx @trancexpress

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.

4 participants