Skip to content

Prevent test suites from being executed when their resources are missing#266

Merged
mdimopoulos merged 1 commit intomasterfrom
missing-test-suites-error_issue232
Jul 25, 2025
Merged

Prevent test suites from being executed when their resources are missing#266
mdimopoulos merged 1 commit intomasterfrom
missing-test-suites-error_issue232

Conversation

@mdimopoulos
Copy link
Copy Markdown
Contributor

@mdimopoulos mdimopoulos commented Jul 25, 2025

Also print out a message to guide the user on how to download test suites

resolves #232

@rsanchez87 rsanchez87 self-requested a review July 25, 2025 10:45
@ylatuya
Copy link
Copy Markdown
Contributor

ylatuya commented Jul 25, 2025 •
edited
Loading

I wouldn't use the refactor category, a refactor modifies code but not the behaviour. I think fix is a better category

ylatuya
ylatuya reviewed Jul 25, 2025
View reviewed changes
@ylatuya ylatuya requested a review from Copilot July 25, 2025 11:11
Copilot AI reviewed Jul 25, 2025
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR prevents test suites from being executed when their required resources are missing from the filesystem. It adds a validation check early in the test suite execution process and provides helpful guidance to users on how to resolve the issue.

  • Adds resource directory validation before test suite execution
  • Provides user-friendly error message with guidance on downloading resources
  • Returns early to prevent execution when resources are unavailable
Comments suppressed due to low confidence (1)

fluster/test_suite.py:527

  • The error message could be more specific about the download command. Consider providing the exact command like fluster.py download --help or the specific download command for this test suite if available.
                f"Please download it first, run `fluster.py --help` for more information"

@mdimopoulos
Copy link
Copy Markdown
Contributor Author

mdimopoulos commented Jul 25, 2025

I wouldn't use the refactor category, a refactor modifies code but not the behaviour. I think fix is a better category

According to https://github.com/angular/angular/blob/22b96b9/CONTRIBUTING.md#type the choice of category is debatable. Even though refactor is not exact it's still more accurate than fix, I am not fixing any bug...

@mdimopoulos mdimopoulos force-pushed the missing-test-suites-error_issue232 branch from badc376 to bf795c7 Compare July 25, 2025 11:25
@ylatuya
Copy link
Copy Markdown
Contributor

ylatuya commented Jul 25, 2025

I wouldn't use the refactor category, a refactor modifies code but not the behaviour. I think fix is a better category

According to https://github.com/angular/angular/blob/22b96b9/CONTRIBUTING.md#type, the choice of category is debatable. Even though refactor is not exact it's still more accurate than fix, I am not fixing any bug...

The category choice is debatable, which is why I am raising this issue. You are definitely not refactoring code because you are changing the behaviour. I would like to understand why do you consider that you are not fixing a bug. A bug doesn't need to be something crashing the software, there are many types of bugs including functional or UX ones.

Test suites can't be run if the resources of the test suites are not available because if they are run, this would result in the problems described in #232: the test suite fails and the failures is a cryptic error code caused by the missing resource.

The current code is running the test suites even though the resources are not there: this is the actual bug. This PR is fixing this bug by doing the correct thing, not running them and advising the users on how to proceed.

@mdimopoulos
Copy link
Copy Markdown
Contributor Author

mdimopoulos commented Jul 25, 2025

I wouldn't use the refactor category, a refactor modifies code but not the behaviour. I think fix is a better category

According to https://github.com/angular/angular/blob/22b96b9/CONTRIBUTING.md#type, the choice of category is debatable. Even though refactor is not exact it's still more accurate than fix, I am not fixing any bug...

The category choice is debatable, which is why I am raising this issue. You are definitely not refactoring code because you are changing the behaviour. I would like to understand why do you consider that you are not fixing a bug. A bug doesn't need to be something crashing the software, there are many types of bugs including functional or UX ones.

Test suites can't be run if the resources of the test suites are not available because if they are run, this would result in the problems described in #232: the test suite fails and the failures is a cryptic error code caused by the missing resource.

The current code is running the test suites even though the resources are not there: this is the actual bug. This PR is fixing this bug by doing the correct thing, not running them and advising the users on how to proceed.

Let's just leave it at that and simply disagree. Even though i am not refactoring code, i am not fixing a bug either.
From my point of view it is an improvement in the workflow of test executions. Currently when a test suite that hasn't been downloaded is executed, the test cases are marked as "error". Such behaviour is not a bug, the value "fail" would be a bug. The closest tag in my opinion would be feat. On the other hand, this change is so small that is not enough for me to label it as a feat.

So i'll just put fix and get on with it.

@mdimopoulos mdimopoulos force-pushed the missing-test-suites-error_issue232 branch from bf795c7 to 62ddf8b Compare July 25, 2025 13:10
@mdimopoulos mdimopoulos merged commit 24e747c into master Jul 25, 2025
4 checks passed
@mdimopoulos mdimopoulos deleted the missing-test-suites-error_issue232 branch July 25, 2025 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ylatuya ylatuya ylatuya left review comments

Copilot code review Copilot Copilot left review comments

@rgonzalezfluendo rgonzalezfluendo rgonzalezfluendo approved these changes

@rsanchez87 rsanchez87 rsanchez87 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Cryptic error message when running without first downloading test vectors

5 participants

@mdimopoulos @ylatuya @rgonzalezfluendo @rsanchez87