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

Sync triangle with problem-specifications #1816

Merged
merged 5 commits into from
Dec 12, 2023
Merged

Sync triangle with problem-specifications #1816

merged 5 commits into from
Dec 12, 2023

Conversation

senekor
Copy link
Contributor

@senekor senekor commented Nov 27, 2023

There was one challenge implementing the test template. The problem specification of this exercise states that it is up to the individual language tracks how to handle invalid triangles. However, this property is not easy to detect from the specification of the test cases. The test cases with invalid triangles were excluded in tests.toml and reimplemented in additional-tests.json with a custom property of "invalid".

It was also necessary to separate the different properties into individual modules. Some descriptions - and therefore the generated test case names - are duplicated across different properties, so they have to be in different namespaces.

There were two challenges implementing the test template.
The problem specification of this exercise states that it is up to the
individual language tracks how to handle invalid triangles and whether
or not to include the floating point cases. However, neither of these
two properties are easy to detect from the specification of the test
cases.

In order to be able to feature-gate the floating point tests, the
word "float" is detected in the test case description. This is not
perfect, but likely robust enough, even if there are more such tests
in the future.

For the invalid triangles, there is no way at all to detect them from
the specification. These test cases had to be excluded in tests.toml
and reimplemented in additional-tests.json with a custom property
of "invalid".

It was also necessary to separate the different properties into
individual modules. Some descriptions - and therefore the generated
test case names - are duplicated across different properties, so
they have to be in different namespaces.
@senekor
Copy link
Contributor Author

senekor commented Nov 27, 2023

In general, the previous test suite seems to have drifted pretty far from problem-specifications. I have noticed two test cases from the previous suite that are now missing, they test whether invalid triangles with floating-point sides are handled correctly. Such a test case does not exist in problem-specifications.

I couldn't think of a case where invalid integer triangles are handled correctly, while invalid floating-point triangles are not, so I think these tests don't provide any value. If I'm missing something and they do provide value, they should probably be upstreamed instead of added to additional-tests.json. There was apparantly an effort to upstream these cases, but the PR was closed due to becoming stale.

The commit introducing these tests on the Rust track state:

For completeness' sake, and just in case someone comes up with a crazy solution

So it seems like the original author also didn't have a concrete scenario in mind where these test cases might provide value.

@ErikSchierboom
Copy link
Member

In order to be able to feature-gate the floating point tests, the
word "float" is detected in the test case description. This is not
perfect, but likely robust enough, even if there are more such tests
in the future.

Maybe it would be worth adding the floating-point scenario to the test cases?

@senekor
Copy link
Contributor Author

senekor commented Dec 9, 2023

Sorry this slipped under the desk a bit. Thanks for suggesting the scenario, I wasn't aware of that feature in canonical-data.json yet. I opened a PR in the problem-spec repo. I'll wait with this PR for that and simplify accordingly.

@senekor senekor merged commit 5916fdf into main Dec 12, 2023
10 checks passed
@senekor senekor deleted the triangle-sync branch December 12, 2023 18:07
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