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

feat: a new simple (correct) test that may break some accepted solutions #242

Closed
wants to merge 2 commits into from

Conversation

barabo
Copy link

@barabo barabo commented Jun 24, 2024

Some of the community solutions incorrectly try to locate STOP codons from input and strip anything that follows the first STOP codon. This is a neat idea, but if the regex is not anchored, or if it does not align to groups of 3 - it's possible that a valid pair of codons (AUGAUG, for example,) creates a pattern that can cause the regex mis-identify a sequence as a STOP codon when it is not (UGA, in the previous example).

  AUGAUG
   ^^^ - not a STOP codon

  UUAAUG
   ^^^ - not a STOP codon

Summary

A couple of the accepted solutions to this problem have a subtle bug that the tests are not catching. This PR corrects that by putting a simple test in place.

Click to see examples...

image

image

Bonus

I noticed that the contractions in the test descriptions were not rendering correctly, so I changed them to non-contractions.
image

Checklist

  • If docs where changed, run ./bin/configlet generate to ensure all documents are properly generated.
  • CI is green

Some of the community solutions incorrectly try to locate STOP codons from input and strip anything that follows the first STOP codon.  This is a neat idea, but if the regex is not anchored, or if it does not align to groups of 3 - it's possible that a valid pair of codons (`AGUAGU`, for example,) creates a pattern that can cause the regex mis-identify a sequence as a STOP codon when it is on (`UAG`, in the previous example).
Copy link

This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested.

If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos.

[no important files changed]

For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping @exercism/maintainers-admin in a comment. Thank you!

@IsaacG
Copy link
Member

IsaacG commented Jun 24, 2024

Thank you for the PR! However, this exercise is a practice exercise so any changes must be applied to the problem specs then synced. Please read https://exercism.org/docs/community/being-a-good-community-member/suggesting-exercise-improvements#h-practice-exercises

@IsaacG IsaacG closed this Jun 24, 2024
@barabo
Copy link
Author

barabo commented Jun 24, 2024

Thanks for the pointer! I opened exercism/problem-specifications#2458 - for cross referencing.

@glennj
Copy link
Contributor

glennj commented Jun 24, 2024

Do you want to submit the jq version of the test now? Use bin/configlet sync and bin/generate_tests

@IsaacG
Copy link
Member

IsaacG commented Jun 24, 2024

Woops. I did a sync PR. Feel free to ignore.

@barabo
Copy link
Author

barabo commented Jun 25, 2024

I'm happy to revive this PR if the changes are still wanted / needed.

@glennj
Copy link
Contributor

glennj commented Jun 25, 2024

Isaac created #243. Do you want to review that, or try to do it yourself?

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.

3 participants