Skip to content

[Hygiene] Remove severity check #1030

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

Merged
merged 2 commits into from
Jun 3, 2025
Merged

Conversation

the-glu
Copy link
Contributor

@the-glu the-glu commented May 27, 2025

Should close #404 at this point :)

Mostly a revert of #968 (expect the rights fix)

@the-glu the-glu requested a review from BenjaminPelletier May 27, 2025 15:55
Copy link
Contributor

@mickmis mickmis left a comment

Choose a reason for hiding this comment

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

Now that everything has been migrated, removing this check sounds good, but we should also IMO avoid introducing in the future any more of those cases.
The easiest way to do so would probably be to remove the severity parameter in record_fail and raise an exception if the severity is not set in the documentation:

def record_failed(
self,
summary: str,
severity: Optional[Severity] = None,
details: str = "",
query_timestamps: Optional[List[datetime]] = None,
additional_data: Optional[dict] = None,
) -> None:
self._outcome_recorded = True
if severity is None:
if "severity" in self._documentation and self._documentation.severity:
severity = self._documentation.severity
else:
raise ValueError(
f"Severity of check '{self._documentation.name}' was not specified at failure time and is not documented in scenario documentation"
)

@the-glu
Copy link
Contributor Author

the-glu commented Jun 2, 2025

Now that everything has been migrated, removing this check sounds good, but we should also IMO avoid introducing in the future any more of those cases. The easiest way to do so would probably be to remove the severity parameter in record_fail and raise an exception if the severity is not set in the documentation:

The problem is that is still used for some legit reasons, like test and severity not in the documentation :/

Would renaming it to something else would do the trick ? Like _severity so it's shouldn't be used by convention :P

@mickmis
Copy link
Contributor

mickmis commented Jun 3, 2025

The problem is that is still used for some legit reasons, like test and severity not in the documentation :/

I cannot find any use of the parameter anywhere else than in unit tests. Am I missing some?
And if it used only in unit tests, then that is not a legit use. Instead the unit tests could specify the severity through the TestCheckDocumentation simulating the fact that the severity was set through the documentation.
WDYT?

@the-glu
Copy link
Contributor Author

the-glu commented Jun 3, 2025

I cannot find any use of the parameter anywhere else than in unit tests. Am I missing some? And if it used only in unit tests, then that is not a legit use. Instead the unit tests could specify the severity through the TestCheckDocumentation simulating the fact that the severity was set through the documentation. WDYT?

Ha correct, mixed up contexts between TestCheckDocumentation and record_failled, will do some cleanup :)

@the-glu the-glu requested a review from mickmis June 3, 2025 12:41
Copy link
Contributor

@mickmis mickmis left a comment

Choose a reason for hiding this comment

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

Nice thanks!

@mickmis mickmis merged commit f7be27e into interuss:main Jun 3, 2025
21 checks passed
@mickmis mickmis deleted the finalize_404 branch June 3, 2025 13:38
github-actions bot added a commit that referenced this pull request Jun 3, 2025
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.

Specify check severity in test scenario documentation only
2 participants