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

Disabled test results #212

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Disabled test results #212

wants to merge 8 commits into from

Conversation

jhughesbiot
Copy link
Collaborator

@jhughesbiot jhughesbiot commented Dec 27, 2023

  • Adds better descriptions for tests that are disabled
  • Detects and ignores disabled tests in report instead of failing \
  • Adds back TLS and DNS tests removed in a disabled state

Add dns and tls tests back in disabled state
@jhughesbiot jhughesbiot changed the base branch from main to dev December 27, 2023 19:39
@jboddey
Copy link
Collaborator

jboddey commented Jan 2, 2024

The output of this PR is as below:
image

If a test is disabled, we do not want it to be present in the test list - especially as the user does not have the ability to enable or disable tests from the UI. Instead, disabled tests should not be present or run at all.

@jhughesbiot
Copy link
Collaborator Author

I think we need to differentiate here what a user can do vs what the UI can do. The user has every capability of disabling tests, the UI does not. Skipping a test within a module and indicating it makes sense. Removing any indication a disabled test exists in the system seems like something that would cause confusion in the results.

@jboddey
Copy link
Collaborator

jboddey commented Jan 3, 2024

Our primary user is manufacturers who use the user interface. With this current setup, we are telling the user about tests which do not exist within qualification (or will not in 4.5). I am not opposed to have the tests available for labs or other users to run but it seems counter productive to have them present.

@jhughesbiot
Copy link
Collaborator Author

Maybe we just need to add this as a configurable option in the report to show/hide disabled tests hiding by default. I can look into that and add to this PR.

Not sure we should remove any tests that might get removed from 4.5 until it's finalized but should continue to use 4.4 as the reference for test generation until then.

@jboddey
Copy link
Collaborator

jboddey commented Jan 4, 2024

Maybe we just need to add this as a configurable option in the report to show/hide disabled tests hiding by default. I can look into that and add to this PR.

Not sure we should remove any tests that might get removed from 4.5 until it's finalized but should continue to use 4.4 as the reference for test generation until then.

With this in mind, I'm happy to have the tests re-added and enabled by default - provided that we add an option in the config to hide disabled tests (prevent them from running at all) which should be enabled by default. Once 4.5 is released, we will set the test status to disabled.

@jboddey jboddey changed the base branch from dev to release/v1.1.1 January 8, 2024 20:48
@jboddey
Copy link
Collaborator

jboddey commented Feb 7, 2024

These re-added tests are no longer present in the device qualification test plan so I will close this PR now. In the future, I would like to add a module with just informational tests that provide us with additional info about devices. I will leave the branch present.

@jboddey jboddey closed this Feb 7, 2024
@jhughesbiot jhughesbiot reopened this Feb 7, 2024
@jhughesbiot
Copy link
Collaborator Author

Re-opening as this is not just adding tests in disabled state, it is fixing disable tests state detection. Regardless of current test plan, we shouldn't remove tests if they are not part of the test plan, we should only ever disable them unless they are fully removed for some other reason.

@jboddey
Copy link
Collaborator

jboddey commented Feb 7, 2024

Re-opening as this is not just adding tests in disabled state, it is fixing disable tests state detection. Regardless of current test plan, we shouldn't remove tests if they are not part of the test plan, we should only ever disable them unless they are fully removed for some other reason.

If this is the case, could we hide disabled tests from the report (with a config option to toggle this behavior - which will be set to hide disabled tests by default)?

@jhughesbiot
Copy link
Collaborator Author

Right now it marks the tests as disabled in their description and with a skipped state. If we want them hidden from the report, we should probably move that to it's own update so we can prevent merge conflicts with all the other recent updates to the report format.

Base automatically changed from release/v1.1.1 to dev February 7, 2024 22:03
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.

None yet

2 participants