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: Tests failing due to disabling JUnitPlatform which is causing 0% coverage reports #1218

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

abubkr-hago
Copy link

@abubkr-hago abubkr-hago commented Aug 18, 2024

New Pull Request Checklist

Issue Description

Some tests are failing after disabling JUnitPlatform.

Closes: #1217.

Approach

Fix failing tests without enabling JUnitPlatform.

TODOs before merging

  • Fix ParseCountingUriHttpBodyTest.testWriteToWithNullOutput
  • Fix ParseCountingUriHttpBodyTest.testWriteTo
  • Fix ParseFileControllerTest.testSaveAsyncSuccessWithUri
  • Fix ParseFileTest.testSaveAsyncSuccessWithUri
  • Fix ParseUriHttpBodyTest.testInitializeWithUri
  • Fix ParseUriHttpBodyTest.testWriteTo
  • Fix ParseCorePluginsTest.testQueryControllerDefaultImpl
  • Fix ParseDecoderTest.testCompleteness
  • Fix ParseDecoderTest.testCompletenessOfIncludedParseObject

Copy link

parse-github-assistant bot commented Aug 18, 2024

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@mtrezza
Copy link
Member

mtrezza commented Aug 18, 2024

I think you have the wrong base for this PR, the base branch does not seem to be up-to-date, hence the file conflicts.

Also, in #1216 I removed

tasks.withType(Test) {
    useJUnitPlatform()
}

But I'm unsure whether the tests before ran on JUnit 5 or 4, because we have these 2 dependencies:

testImplementation "org.junit.jupiter:junit-jupiter:5.6.0"
testImplementation "junit:junit:4.13.2"

Maybe we were using JUnit 5, so removing useJUnitPlatform now causes the tests to fail, even though they only fail because we now run on JUnit 4 instead of 5. So maybe the solution is not to fix the tests, but to find out why useJUnitPlatform did not execute the tests properly?

@abubkr-hago
Copy link
Author

I think you have the wrong base for this PR, the base branch does not seem to be up-to-date?

Yes, I continued working on the same branch.
I'll merge them once all tests pass.

@abubkr-hago
Copy link
Author

Maybe we were using JUnit 5, so removing useJUnitPlatform now causes the tests to fail, even though they only fail because we now run on JUnit 4 instead of 5. So maybe the solution is not to fix the tests, but to find out why useJUnitPlatform did not execute the tests properly?

This seems to be a better solution, downgrading JUnit would probably cause more issues later on.

@mtrezza
Copy link
Member

mtrezza commented Aug 18, 2024

I'm not sure whether the PR has effectively downgraded it, because of the 2 dependencies. Do you see which JUnit version the tests are running with?

@abubkr-hago
Copy link
Author

I'm not sure whether the PR has effectively downgraded it, because of the 2 dependencies. Do you see which JUnit version the tests are running with?

It did, the current version is 4.13.2, you can print it using junit.runner.Version.id().

@abubkr-hago
Copy link
Author

abubkr-hago commented Aug 18, 2024

Good News, I got a 1% coverage report after refactoring imports to org.junit.jupiter.api in one of the test files while useJUnitPlatoform is enabled.

It seems we will need to refactor all test files to use org.junit.jupiter instead of org.junit.

@abubkr-hago abubkr-hago force-pushed the fix-failing-tests branch 4 times, most recently from 4c9de42 to fdce770 Compare August 19, 2024 06:14
@abubkr-hago
Copy link
Author

abubkr-hago commented Aug 19, 2024

Good News, I got a 1% coverage report after refactoring imports to org.junit.jupiter.api in one of the test files while useJUnitPlatoform is enabled.

It seems we will need to refactor all test files to use org.junit.jupiter instead of org.junit.

It turns out only one runtime can be used when testing, meaning this and the previous builds were running JUnit4 all the time.
But now JUnitPlatform doesn't ignore JUnit4 tests.
Also, we can't upgrade to JUnit5 since Robolectric doesn't support JUnit5 jupiter runtime.

@mtrezza
Copy link
Member

mtrezza commented Aug 19, 2024

Interesting; so what is the solution? Stick with JUnit 4 for now? Does it require a lot of changes?

@abubkr-hago
Copy link
Author

abubkr-hago commented Aug 20, 2024

Interesting; so what is the solution? Stick with JUnit 4 for now? Does it require a lot of changes?

I think we will have to stick with JUnit4 for now, coverage is still not reporting though, look at Run tests here. I'll fix this ASAP.

It doesn't require a lot of changes, I'll just have to fix the remaining errors.

@mtrezza
Copy link
Member

mtrezza commented Aug 22, 2024

Sounds good; could you remove the unrelated changes from this PR, so we can merge it?

@abubkr-hago
Copy link
Author

Sounds good; could you remove the unrelated changes from this PR, so we can merge it?

Do you mean remove the changes related to upgrading different packages? and keep the changes related to fixing the failing tests?

There are still 3 failing tests I'm trying to figure out:

  • Fix ParseCorePluginsTest.testQueryControllerDefaultImpl
  • Fix ParseDecoderTest.testCompleteness
  • Fix ParseDecoderTest.testCompletenessOfIncludedParseObject

@mtrezza
Copy link
Member

mtrezza commented Aug 24, 2024

Do you mean remove the changes related to upgrading different packages? and keep the changes related to fixing the failing tests?

Yes, only keep changes related to fixing coverage. Other changes should move into their dedicated PRs.

@abubkr-hago
Copy link
Author

junit-vintage-engine runtime fixes 0% coverage reporting issue, should I include fixing the failing tests in this PR?

This PR mentions this issue #1217.

@mtrezza
Copy link
Member

mtrezza commented Sep 4, 2024

Oh nice, with that, can we mix Junit 4 and 5 tests? Because at some point I guess we'll have to start migrating, or maybe require newly added tests to be written already in JUnit 5.

@abubkr-hago
Copy link
Author

abubkr-hago commented Sep 10, 2024

Yes both JUnit4 and Jupiter (JUnit5) tests can be mixed now.

@mtrezza
Copy link
Member

mtrezza commented Sep 10, 2024

Very nice!

@parse-community parse-community deleted a comment from KutayKerem Sep 15, 2024
@mtrezza
Copy link
Member

mtrezza commented Sep 15, 2024

Could you take a look at the failing CI?

Edit: Increased the lint timeout from 5 to 10 mins.

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.

Fix failing tests
2 participants