Skip to content

test: reproduction case for #9365#9366

Merged
rgrinberg merged 1 commit intoocaml:mainfrom
Alizter:ps/branch/test__reproduction_case_for__9365
Dec 5, 2023
Merged

test: reproduction case for #9365#9366
rgrinberg merged 1 commit intoocaml:mainfrom
Alizter:ps/branch/test__reproduction_case_for__9365

Conversation

@Alizter
Copy link
Copy Markdown
Collaborator

@Alizter Alizter commented Dec 3, 2023

We also test the other stanzas while reproducing the bug with the test stanza.

@Alizter
Copy link
Copy Markdown
Collaborator Author

Alizter commented Dec 3, 2023

I have a fix ready after we merge this.

@Alizter Alizter force-pushed the ps/branch/test__reproduction_case_for__9365 branch from b5ff009 to 4ecc9a7 Compare December 4, 2023 04:56
@rgrinberg
Copy link
Copy Markdown
Member

If the issue is about the test stanza, why does the test include library and executable stanzas? Shouldn't it be possible to reproduce the issue only with the test stanza?

@Alizter
Copy link
Copy Markdown
Collaborator Author

Alizter commented Dec 4, 2023

@rgrinberg It's just about the test stanza, but this ensures we are also testing the library and executable stanzas.

@rgrinberg
Copy link
Copy Markdown
Member

Don't we have other select tests that make this redundant?

@Alizter
Copy link
Copy Markdown
Collaborator Author

Alizter commented Dec 4, 2023

I could only find a test for executable, this additionally has a test for library. If you like, I can reduce this test down to test and add an additional library test. (Although I think it makes more sense to test them in one place).

@rgrinberg
Copy link
Copy Markdown
Member

It would be better not to mix adding a regression test and re-organizing the existing tests. So I suggest that you a regression test and then create a subsequent PR to re-organize things.

We also test the other stanzas while reproducing the bug with the test
stanza.

Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter Alizter force-pushed the ps/branch/test__reproduction_case_for__9365 branch from 4ecc9a7 to 38800da Compare December 4, 2023 23:38
@Alizter
Copy link
Copy Markdown
Collaborator Author

Alizter commented Dec 4, 2023

I've reduced the test down to testing only the bugged behaviour of select in test.

@rgrinberg
Copy link
Copy Markdown
Member

By the way, there might a similar bug with melange.emit

@rgrinberg
Copy link
Copy Markdown
Member

Thanks. Next time, try to keep the test demonstrating the bug as simple as possible. Improvements can always be made later.

@rgrinberg rgrinberg merged commit aa5d6df into ocaml:main Dec 5, 2023
@Alizter Alizter deleted the ps/branch/test__reproduction_case_for__9365 branch December 5, 2023 00:15
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.

2 participants