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.

Sign up for GitHub

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

Jump to bottom

Correct matchesJsonValue #17953

Merged
merged 1 commit into from
Feb 10, 2025
Merged

Correct matchesJsonValue #17953

merged 1 commit into from
Feb 10, 2025

Conversation

Arthur-Milchior
Copy link
Member

expectedMap used to associated the keys of the expectedValue to the values of actualValue.

As differtKeys also looked at the values in actualValue, it means the values of expectedValues were always ignored.

I hope that this new writting, checking for the equality of the set of keys, and then for the equalities of value, will be more readable and hence improve the test suite.

Luckily enough we only had one failing test. I corrected the expected value in the test. As we use the default filtered deck provided by the back-end I don't expect the implementation to be buggy here.

@Arthur-Milchior
Correct matchesJsonValue
e5e3b48 
`expectedMap` used to associated the keys of the `expectedValue` to the
values of `actualValue`.

As `differtKeys` also looked at the values in `actualValue`, it means
the values of `expectedValues` were always ignored.

I hope that this new writting, checking for the equality of the set of
keys, and then for the equalities of value, will be more readable and
hence improve the test suite.

Luckily enough we only had one failing test. I corrected the expected
value in the test. As we use the default filtered deck provided by the
back-end I don't expect the implementation to be buggy here.
david-allison
david-allison approved these changes Feb 10, 2025
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Great catch!

@david-allison david-allison added the Needs Second Approval Has one approval, one more approval to merge label Feb 10, 2025
mikehardy
mikehardy approved these changes Feb 10, 2025
View reviewed changes
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Well that's unexpected. Good catch, good fix

@mikehardy mikehardy added this pull request to the merge queue Feb 10, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 10, 2025
@mikehardy
Copy link
Member

Flake? Not sure if it's a "test process" flake or a code issue.


ExportDialogFragmentTest > Legacy export checkbox(default false) is shown only for collection and apkg FAILED
    java.lang.RuntimeException: Interrupted while interacting
        at androidx.test.espresso.InteractionResultsHandler.gatherAnyResult(InteractionResultsHandler.java:88)
        at androidx.test.espresso.InteractionResultsHandler.gatherAnyResult(InteractionResultsHandler.java:52)
        at androidx.test.espresso.ViewInteraction.waitForAndHandleInteractionResults(ViewInteraction.java:383)
        at androidx.test.espresso.ViewInteraction.desugaredPerform(ViewInteraction.java:212)
        at androidx.test.espresso.ViewInteraction.perform(ViewInteraction.java:140)
        at androidx.test.espresso.DataInteraction.perform(DataInteraction.java:137)
        at com.ichi2.anki.export.ExportDialogFragmentTest.Legacy_export_checkbox_default_false__is_shown_only_for_collection_and_apkg$lambda$0(ExportDialogFragmentTest.kt:60)
        at androidx.fragment.app.testing.FragmentScenario.onFragment$lambda$6(FragmentScenario.kt:322)
        at androidx.test.core.app.ActivityScenario.lambda$onActivity$2(ActivityScenario.java:794)
        at androidx.test.core.app.ActivityScenario.onActivity(ActivityScenario.java:804)
        at androidx.fragment.app.testing.FragmentScenario.onFragment(FragmentScenario.kt:315)
        at com.ichi2.anki.export.ExportDialogFragmentTest.Legacy export checkbox(default false) is shown only for collection and apkg(ExportDialogFragmentTest.kt:40)

        Caused by:
        java.lang.InterruptedException
            at java.base/java.util.concurrent.locks.ReentrantLock$Sync.lockInterruptibly(ReentrantLock.java:159)
            at java.base/java.util.concurrent.locks.ReentrantLock.lockInterruptibly(ReentrantLock.java:372)
            at java.base/java.util.concurrent.LinkedBlockingQueue.take(LinkedBlockingQueue.java:432)
            at androidx.test.espresso.InteractionResultsHandler.gatherAnyResult(InteractionResultsHandler.java:83)
            ... 11 more

@mikehardy mikehardy added this pull request to the merge queue Feb 10, 2025
Merged via the queue into ankidroid:main with commit 36e3631 Feb 10, 2025
9 checks passed
@github-actions github-actions bot added this to the 2.21 release milestone Feb 10, 2025
@github-actions github-actions bot removed the Needs Second Approval Has one approval, one more approval to merge label Feb 10, 2025
@Arthur-Milchior Arthur-Milchior deleted the isJsonEqual branch February 10, 2025 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikehardy mikehardy mikehardy approved these changes

@david-allison david-allison david-allison approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.21 release
Development

Successfully merging this pull request may close these issues.
3 participants
@Arthur-Milchior @mikehardy @david-allison