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

[Unit Testing]: More test cases #647

Closed
wants to merge 9 commits into from

Conversation

arthurscchan
Copy link
Contributor

@arthurscchan arthurscchan commented Nov 30, 2022

Add more unit testing test cases.

Related to issue #630 and extending PR #643

Signed-off-by: Arthur Chan [email protected]

@arthurscchan arthurscchan force-pushed the more-test-cases branch 6 times, most recently from 5c3693b to 06f085e Compare November 30, 2022 22:44
Arthur Chan added 7 commits November 30, 2022 22:45
Signed-off-by: Arthur Chan <[email protected]>
Signed-off-by: Arthur Chan <[email protected]>
Signed-off-by: Arthur Chan <[email protected]>
@arthurscchan arthurscchan changed the title [JVM issue]: More test cases [Unit Testing]: More test cases Nov 30, 2022
}

@Test
public void sampleTestCases() throws IOException, InterruptedException {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this test. The problem is that it may become a somewhat blocker of progress. Down the line, if we start to extend how the output looks, then this will fail even though semantics are intact. I feel like the tests should be rooted in semantic rather than syntax -- and this one compares two outputs in a string comparison fashion.

I think the easiest approach is to make the test a bit more narrow and focus on making the tests check that certain properties hold.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. And new unitesting has been pushed in PR #663

@arthurscchan
Copy link
Contributor Author

Not needed anymore, using another approach in PR #663

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