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

Allow valid agent types in PCT context #660

Merged
merged 2 commits into from
Jan 3, 2024
Merged

Conversation

basil
Copy link
Member

@basil basil commented Aug 24, 2023

While trying to add the Kubernetes plugins to jenkinsci/bom in jenkinsci/bom#2417 I got some test failures of the form:

"error": "Invalid agent type "foo" specified. Must be one of [any, kubernetes, label, none, otherField]"
}]) didn't contain expected error 'Invalid agent type "foo" specified. Must be one of [any, label, none, otherField]'

The existing test logic had tried to deal with this case by defining a legalAgentTypes field, but it was flawed in a few ways:

  • It was not used consistently in AbstractModelDefTest#configsWithErrors: unknownBareAgentType used it but not unknownAgentType
  • Any usage of legalAgentTypes from AbstractModelDefTest#configsWithErrors was flawed and yielding the empty string (thus making the test pass erroneously) because Jenkins had not been started when populating the list during JUnit parameterized test initialization
  • It did not assert that the types expected to be contributed by this plugin itself were present

This PR fixes all three mistakes by:

  • Consistently not using legalAgentTypes in AbstractModelDefTest#configsWithErrors, since that is too early for it to be used
  • Making the field non-static so that it cannot be used too early by accident in the future
  • Ensuring that any tests that need legalAgentTypes use it in the test itself, after Jenkins startup (currently this list includes ErrorsEndpointOpsTest, ErrorsJSONParserTest, and ValidatorTest)
  • Asserting that the types expected to be contributed by this plugin are present

Testing done

To test this PR I reproduced the original problem by adding the Kubernetes plugins to the test classpath and verified the BOM error occurred. Next I applied the changes from this PR and verified the problem was fixed. Then I removed the Kubernetes plugins from the test classpath and verified the tests still passed in the non-PCT scenario.

@@ -223,7 +226,7 @@ public static Iterable<Object[]> configsWithErrors() {
result.add(new Object[]{"invalidWrapperType", "Invalid option type \"echo\". Valid option types:"});
result.add(new Object[]{"invalidStageWrapperType", "Invalid option type \"echo\". Valid option types:"});

result.add(new Object[]{"unknownBareAgentType", Messages.ModelValidatorImpl_InvalidAgentType("foo", legalAgentTypes)});
result.add(new Object[]{"unknownBareAgentType", Messages.ModelValidatorImpl_InvalidAgentType("foo", "[any, label, none, otherField]")});
Copy link
Member Author

Choose a reason for hiding this comment

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

Newly consistent with unknownAgentType in the sense that both now hardcode a list of agent types rather than using legalAgentTypes, which is not yet initialized when this code is running.

@basil
Copy link
Member Author

basil commented Aug 29, 2023

Currently blocking jenkinsci/bom#2417

@basil
Copy link
Member Author

basil commented Sep 1, 2023

Currently blocking jenkinsci/bom#2453

@basil
Copy link
Member Author

basil commented Nov 28, 2023

Currently blocking jenkinsci/bom#2453

@jglick jglick merged commit 04286e2 into jenkinsci:master Jan 3, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants