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

[20] Fix and enable the commented out tests in SwitchPatternTest #932

Closed
jarthana opened this issue Mar 31, 2023 · 7 comments
Closed

[20] Fix and enable the commented out tests in SwitchPatternTest #932

jarthana opened this issue Mar 31, 2023 · 7 comments
Assignees

Comments

@jarthana
Copy link
Member

As many as 19 tests were commented out during pull #556. If the tests indeed need to be fixed, we should do that and capture the new behavior in those tests rather than just commenting them out.

@mpalat please take a look.

@jarthana jarthana added this to the 4.28 milestone Mar 31, 2023
@mpalat
Copy link
Contributor

mpalat commented Mar 31, 2023

@jarthana please take a look at #635 - As you can see, this issue was created separately to address these failures. There is one omission - thanks for pushing for a relook - have raised #933 to address that.

And here are the details for the rest of the ones:
574719 - 6 - grammar change - this is no longer relevant.
575241 - 3 - already enabled
575356 - 2 - grammar change - Only one test is relevant - that is enabled - other one disabled
575047 - 1 - already enabled
575051 - 1 - not relevant with the fourth preview - hence disabled
578553 - 1 - already enabled
556 - 2 - already enabled

@mpalat mpalat closed this as completed Mar 31, 2023
@jarthana
Copy link
Member Author

Here's why I think they are relevant (at least the testBug574719* ones)

The test code now fails with the following compiler error:

A 'default' can occur after 'case' only as a second case label expression and that too only if 'null' precedes in 'case null, default'

This is a new error message that was introduced by the PR I mentioned. Any guess how many tests test this error message?

  • Zero.

I think even other tests can be made relevant by adjusting them to test the new behavior. Last resort should be removing them, instead of leaving lot of stale code.

@mpalat
Copy link
Contributor

mpalat commented Mar 31, 2023

This is a new error message that was introduced by the PR I mentioned. Any guess how many tests test this error message?

  • Zero.

@jarthana - that observation is wrong. Check testIssue_556_003

@jarthana
Copy link
Member Author

@jarthana - that observation is wrong. Check testIssue_556_003

My bad. I didn't account for the escaping. In that case, the tests that are no longer relevant can be removed if you are sure these are covered elsewhere.

@mpalat
Copy link
Contributor

mpalat commented Mar 31, 2023

@jarthana - that observation is wrong. Check testIssue_556_003

My bad. I didn't account for the escaping. In that case, the tests that are no longer relevant can be removed if you are sure these are covered elsewhere.

@jarthana Thanks! However, am just holding on to these commented tests until the Switch Pattern is standardized - expected to be in 21 so that if we have some of these becoming relevant again, we can just enable them easily. We can remove them once it is standardized. Please bear with this until then.

@jarthana jarthana removed this from the 4.28 milestone Mar 31, 2023
@jarthana
Copy link
Member Author

Sure, then let's keep this issue so we can track this. I will remove the milestone.

@jarthana
Copy link
Member Author

I have enabled and fixed some tests and deleted some that are no longer relevant. Closing now.

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

No branches or pull requests

2 participants