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

Fix JUnit Parameterized Tests #2538

Open
8 of 13 tasks
tenthe opened this issue Mar 12, 2024 · 18 comments
Open
8 of 13 tasks

Fix JUnit Parameterized Tests #2538

tenthe opened this issue Mar 12, 2024 · 18 comments
Labels
good first issue Good for newcomers
Milestone

Comments

@tenthe
Copy link
Contributor

tenthe commented Mar 12, 2024

Description

Due to the update from JUnit4 to JUnit5, we had to deactivate some tests that do not use the JUnit5 syntax for parameterized tests. Below you will find a list of these tests that should be migrated step by step.

To see an example, I have started to migrate a first test (see TestBooleanFilterProcessor).

If you are interested in working on one of the tests, please let us know and we will create an issue for each test and assign it to you.

List of classes

@tenthe tenthe changed the title Fix Parameterized Tests Fix JUnit Parameterized Tests Mar 12, 2024
@tenthe tenthe added the good first issue Good for newcomers label Mar 12, 2024
@bossenti bossenti added this to the 0.97.0 milestone Mar 12, 2024
@nine03
Copy link

nine03 commented Mar 13, 2024

Hi, i want to work on this issue.
TestBooleanCounterProcessor Could you assign this issue?

@tenthe
Copy link
Contributor Author

tenthe commented Mar 13, 2024

Hi @nine03,
Thank you for your interest.
I have created an issue for you. Can you please comment under it so that I can assign it to you.
If you have any questions, please let us know.

Cheers,
Philipp

@pambrocio
Copy link

currently looking at TestSizeMeasureProcessor what are the values for these parameters
@Parameterized.Parameter
public String sizeUnit;

@Parameterized.Parameter(1)
public int numOfBytes;

@Parameterized.Parameter(2)
public double expectedSize;

@Parameterized.Parameter(3)
public double allowableError;

@pambrocio
Copy link

Is it ok to use enums for this?? do we just need paremeterized inputs for org.apache.streampipes.processors.enricher.jvm.processor.sizemeasure.TestSizeMeasureProcessor#testSizeMeasureProcessor, (this is currently commented) . And Just make the test work???

@pambrocio
Copy link

would like to take on TestStringTimerProcessor

@bossenti
Copy link
Contributor

Here you go: #2764 🙂

@pambrocio
Copy link

pambrocio commented Apr 24, 2024

please assign the remaining Test classes to me

@bossenti
Copy link
Contributor

@pambrocio issues are created (see at the top)
I can assign you once you wrote a comment there

@pambrocio
Copy link

Thanks @bossenti

@bossenti
Copy link
Contributor

Hey @pambrocio,

I just realized that some tests you have created PRs for are also part of #2375 (for reference: #2737, #2374).
Would be great if we could align both endeavors to avoid extra work.
Sorry for not pointing out earlier.

@pambrocio
Copy link

pambrocio commented Apr 25, 2024

Hi @bossenti ,

Too bad i am almost done only one class left. Shall i continue?? this is the remaining class i have not migrated #2779 TestStringToStateProcessor

@pambrocio
Copy link

@tenthe

Any advice on this?

@pambrocio
Copy link

Hi @tenthe @bossenti @IsaakKrut,

the remaining classes have been migrated to Junit 5, Tell me what needs to be done to get my work merged to the main branch.

Thanks,
Pambrocio

@bossenti
Copy link
Contributor

Hi @bossenti ,

Too bad i am almost done only one class left. Shall i continue?? this is the remaining class i have not migrated #2779 TestStringToStateProcessor

Actually, I already mentioned that earlier: #2754 (comment)

Would you be interested to contribute to #2375 by adapting your migrated tests to the framework introduced there and help to improve it?

If not, we can merge the migrated tests that are not part of #2375. Otherwise, we would cause a lot of migration effort in #2375.

@pambrocio
Copy link

Hi @bossenti,

Would love to. Let me check the discussion

@bossenti
Copy link
Contributor

bossenti commented May 2, 2024

@pambrocio would be great if you are willing to continue your work based on #2375 (comment) 🙂

@pambrocio
Copy link

@bossenti, I am quite unclerar on what code changes need to be implemented

@bossenti
Copy link
Contributor

bossenti commented May 2, 2024

In #2375 we introduced a standardized approach how pipeline elements can (and should) be tested in future (https://github.com/apache/streampipes/pull/2375/files#diff-a8a7ae4c9654c4422ef00768c7887a92bfffc6e0726c1922531f57ae865ee531).
The PR also contains some examples how this approach can be applied, e.g., https://github.com/apache/streampipes/pull/2375/files#diff-49a3517e7b83d0021ebf95c11e120d451297aff1c3eea3aab3f43dc8042cf9f0

So the task now is to update your PRs accordingly. This includes updating your branch with the recent changes in the dev branch and change the implementation of the tests so that they use the above described approach. As @tenthe already said, this approach is not fixed yet, so if you have any feedback, improvements we are happy to hear and address them or you can directly improve the proposed approach.

Does that make it clearer? 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants