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

SerializationTests::Actions started failing recently #13323

Closed
j4james opened this issue Jun 18, 2022 · 3 comments · Fixed by #13603
Closed

SerializationTests::Actions started failing recently #13323

j4james opened this issue Jun 18, 2022 · 3 comments · Fixed by #13603
Assignees
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@j4james
Copy link
Collaborator

j4james commented Jun 18, 2022

Windows Terminal version

Commit 848314e

Windows build number

10.0.19044.1706

Other Software

No response

Steps to reproduce

  1. Build a recent commit of OpenConsole (I think anything after 799b5d4 should do).
  2. Run the unit tests.

Expected Behavior

All the tests should pass.

Actual Behavior

There is a failure in SettingsModelLocalTests::SerializationTests::Actions when attempting to serialize the pair of findMatch actions. It seems that the commands are being reordered.

Click to see error
Error: Verify: AreEqual(toString(json), toString(result)) - Values ([
        {
                "command" :
                {
                        "action" : "findMatch",
                        "direction" : "next"
                },
                "keys" : "ctrl+shift+s"
        },
        {
                "command" :
                {
                        "action" : "findMatch",
                        "direction" : "prev"
                },
                "keys" : "ctrl+shift+r"
        }
], [
        {
                "command" :
                {
                        "action" : "findMatch",
                        "direction" : "prev"
                },
                "keys" : "ctrl+shift+r"
        },
        {
                "command" :
                {
                        "action" : "findMatch",
                        "direction" : "next"
                },
                "keys" : "ctrl+shift+s"
        }
])

I'm not sure if this is actually an indication of a bug - is the order of the commands expected to be retained? But at the very least I'd expect the test to be corrected if this is just an altered hash map order or something of that sort.

If it helps, I think the first time the test stopped working was after commit 799b5d4.

@j4james j4james added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Jun 18, 2022
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jun 18, 2022
@zadjii-msft
Copy link
Member

If it helps, I think the first time the test stopped working was after commit 799b5d4.

That's w e i r d, Cause, that shouldn't really affect this at all 🙃

I don't think the order really matters at all tbh, string comparing the json was just a shortcut vs having to actually re-parse and structurally analyze for equality. Let's just swap the order around (if it's seemingly stable in the new order)

@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Jun 20, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 20, 2022
@zadjii-msft zadjii-msft added this to the Terminal v1.15 milestone Jun 20, 2022
@zadjii-msft zadjii-msft self-assigned this Jun 20, 2022
@zadjii-msft
Copy link
Member

I've got a fix in, well... 5a40e80 I think. I think I accidentally did a git add --all there, so it's a bit tangled with other prototypes. I'll submit the PR tomorrow.

@zadjii-msft
Copy link
Member

Note to self: dev/migrie/b/12788-did-it-work. That's the branch where I tried to do the CI changes that really did not work.

zadjii-msft added a commit that referenced this issue Jul 26, 2022
@ghost ghost added the In-PR This issue has a related PR label Jul 26, 2022
@ghost ghost closed this as completed in #13603 Jul 27, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jul 27, 2022
ghost pushed a commit that referenced this issue Jul 27, 2022
Cleans up a couple local test failures.

* [x] Closes #13474: So, I clearly hadn't ran the local tests at the end of the themes PR. We needed a sensible fallback to SOME theme, even if there wasn't one provided in the user json. This is only really hit in the tests (that don't also include `defaults.json`.
* [x] Closes #13323: Meh, the ordering of the keys in this test doesn't matter. Ordering is a map implementation detail. This is fine.
* [x] Ran tests locally
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants