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.

@mtrezza
Copy link
Member

mtrezza commented Oct 5, 2024

Re-running the CI, but it fails. @abubkr-hago do you remember how we wanted to proceed here? There were a couple of PRs open.

@abubkr-hago
Copy link
Author

Sorry for the late response, I got a job and got very busy.
I actually couldn't find what these errors are coming from. Is it because of the JVM or the OS of github VMs.

@mtrezza
Copy link
Member

mtrezza commented Oct 31, 2024

If we could get this PR over the finish line, we could also merge #1213; @azlekov do you maybe have any time to take a look at why this is failing? It would be a shame these 2 PRs got stale

@azlekov
Copy link
Contributor

azlekov commented Nov 1, 2024

Hello, @mtrezza let's have first the #1213 merged. Probably after that the project can be configured via https://github.com/mannodermaus/android-junit5 this plugin to have both JUnit 4 and JUnit 5. This plugin requires AGP to be higher then 8.0.0

Another approach would be to downgrade the tests to JUnit 4 but I remember when I was doing some version bump I migrate to JUnit 5 because I thought it should be supported well and because of some issues with the tests. Looks like JUnit 4 is still preferable for Android setup.

@mtrezza
Copy link
Member

mtrezza commented Nov 1, 2024

Oh the intention was the other was around I believe. The other one has codecov not working anymore, so this one was opened. This should already support both JUnit versions.

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
3 participants