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

Fix largest-series-products tests #623

Merged
merged 1 commit into from
Jun 14, 2023
Merged

Fix largest-series-products tests #623

merged 1 commit into from
Jun 14, 2023

Conversation

kytrinyx
Copy link
Member

We deleted a couple of tests from the test suite
but they were still being referenced from allTests.

This deletes the stray references.

We deleted a couple of tests from the test suite
but they were still being referenced from allTests.

This deletes the stray references.
kytrinyx referenced this pull request Jun 14, 2023
* Sync largest-series-product docs with problem-specifications

The largest-series-product exercise has been overhauled as part of a project
to make practice exercises more consistent and friendly.

For more context, please see the discussion in the forum, as well as
the pull request that updated the exercise in the problem-specifications
repository:

- https://forum.exercism.org/t/new-project-making-practice-exercises-more-consistent-and-human-across-exercism/3943
- exercism/problem-specifications#2246

* Delete test cases from largest-series-product

This deletes two deprecated test cases so that we can
dramatically simplify the instructions for this exercise.
@kytrinyx kytrinyx requested a review from ErikSchierboom June 14, 2023 08:43
@meatball133
Copy link
Member

meatball133 commented Jun 14, 2023

I think the old test runner had a reference to the allTests.
But the new one removed that reference, so that constant is not really needed, so it could be omitted.

@ErikSchierboom ErikSchierboom merged commit 83f2285 into main Jun 14, 2023
@ErikSchierboom ErikSchierboom deleted the fix-test-deletion branch June 14, 2023 09:16
@kytrinyx
Copy link
Member Author

That would explain why CI passed 😅

@kytrinyx
Copy link
Member Author

@meatball133 Is allTests just a concept exercises thing? There are a bunch of these constants everywhere.

@meatball133
Copy link
Member

meatball133 commented Jun 14, 2023

I will be honest, I am not sure. But I think there were some kind of requirement to have it since swift couldn't find tests by itself and then Apple made so you didn't anymore, I think this pr added it for linux: swiftlang/swift-package-manager#2174

Here can you see one of apples repo updating its tests files: https://github.com/apple/swift-syntax/pull/171/files

@kytrinyx
Copy link
Member Author

So... maybe we can remove them everywhere. I will make a PR and we can see what happens.

@meatball133
Copy link
Member

meatball133 commented Jun 14, 2023

Concept exercises have 2 additional files.
I have removed those files and the alltest from the test runner and the exercise still works.
But removing testAll without removing those 2 files will create errors.

But yes I think we could try to remove them.

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