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

UPDATE test cases from problem specifications for prime-factors #895

Merged
merged 5 commits into from
Aug 14, 2024

Conversation

JagritGumber
Copy link
Contributor

  • Updated all the test cases including new ones added
  • Regenerated test.toml with configlet
  • Configlet test says all filepaths, metadata, and tests for prime-factors are up to date

- Updated all the test cases including new ones added
- Regenerated test.toml with configlet
- Configlet test says all filepaths, metadata, and tests for prime-factors are up to date
Copy link
Contributor

github-actions bot commented Aug 5, 2024

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

Copy link
Contributor

github-actions bot commented Aug 5, 2024

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!

@github-actions github-actions bot closed this Aug 5, 2024
@JagritGumber JagritGumber mentioned this pull request Aug 5, 2024
21 tasks
@vaeng vaeng reopened this Aug 5, 2024
@JagritGumber
Copy link
Contributor Author

Missed some semicolons and formatting will fix that right away

@JagritGumber
Copy link
Contributor Author

The issue is specifically with the last test case, and I am unsure as per what to do with this, because the number is a long and the test cases need to be int from my understanding
image
The clang fails up because the number ends up to be some negative number

@vaeng
Copy link
Contributor

vaeng commented Aug 13, 2024

According to the test.toml, the "large prime" test is not new. That might have been a mistake, as you are the one who included the test just now.

I see three options:

  1. skipping the test, as it does not fit the current status quo. The old example solution is for int type, thus, it would not allow such a big number. This has the advantage that old solutions are still valid.
  2. adding the test, but changing the number to something below 2^31. This would be a "big number", but I am not sure it is in the spirit of the exercise.
  3. changing the example solution from int to long. This would allow us to keep to the exercise spec, but old solutions might become invalid.

I prefer option 3. Number 2 is "more of the same" as the other test.

Do you have an opinion @siebenschlaefer @ErikSchierboom?

@siebenschlaefer
Copy link
Contributor

siebenschlaefer commented Aug 13, 2024

The issue is specifically with the last test case, and I am unsure as per what to do with this, because the number is a long and the test cases need to be int from my understanding.

I don't think that int is required by the instructions or the tests.
To be able to implement this test with the 37-bit number 93819012551 we could change to long long or std::int64_t.
But the real problem is that it would invalidate all existing solutions when we change the expected result from std::vector<int> to std::vector<long long>.

I see three options: [...]

I agree.
I think option three is the best one in the long run because we can implement all tests without deviating from the problem specification.
If we don't want to invalidate old solutions I would vote for option two with something like 11 * 13901 * 14033 == 2145800063 because that's at least somewhat large.

@vaeng
Copy link
Contributor

vaeng commented Aug 14, 2024

If we don't want to invalidate old solutions

That would touch ~1300 solutions. I could live with that, the fix is easy for everyone who understands the widening from int to long long. So if they want to fix it, it would not cost them a lot of time.

@JagritGumber
Copy link
Contributor Author

Well if it is good with long long, I can work on updating the test cases to rather support long long instead of int, Because adding more test cases in this problem and we would encounter this problem sooner or later.

@vaeng
Copy link
Contributor

vaeng commented Aug 14, 2024

Well if it is good with long long, I can work on updating the test cases to rather support long long instead of int, Because adding more test cases in this problem and we would encounter this problem sooner or later.

You would also need to update the source and header files accordingly in the .meta directory. Is that okay?

@JagritGumber
Copy link
Contributor Author

Well if it is good with long long, I can work on updating the test cases to rather support long long instead of int, Because adding more test cases in this problem and we would encounter this problem sooner or later.

You would also need to update the source and header files accordingly in the .meta directory. Is that okay?

Yup, seems good! I see the examples use int too, I'll update that. I think it could work as a clear example of what's been changed.

@ErikSchierboom
Copy link
Member

I'm fine with option 3, just as long as the PR contains [no important files changed] in the commit message to prevent re-testing of all the existing solutions. See https://exercism.org/docs/building/tracks#h-avoiding-triggering-unnecessary-test-runs

Updated examples to also use long long instead of int
[no important files changed]
@vaeng vaeng added the x:module/practice-exercise Work on Practice Exercises label Aug 14, 2024
Copy link
Contributor

@vaeng vaeng left a comment

Choose a reason for hiding this comment

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

Good work. Thanks a lot. 🚀

@vaeng vaeng merged commit af6faab into exercism:main Aug 14, 2024
8 checks passed
@JagritGumber JagritGumber deleted the feat--update-prime-factors-tests branch August 14, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:module/practice-exercise Work on Practice Exercises
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants