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.

Sign up for GitHub

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

Jump to bottom

Improve test config change handling (#164) #177

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Improve test config change handling (#164) #177

wants to merge 12 commits into from
+488 −53

Conversation

MarioFuchsTT
Copy link
Collaborator

Resolves #164

@MarioFuchsTT MarioFuchsTT self-assigned this Sep 12, 2024
@MarioFuchsTT MarioFuchsTT added the documentation Improvements or additions to documentation label Sep 12, 2024
@MarioFuchsTT MarioFuchsTT force-pushed the i164_improve-test-config-change-handling branch from f59a966 to 92b5f33 Compare October 4, 2024 10:32
MartinGroscheTT
MartinGroscheTT requested changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@MartinGroscheTT MartinGroscheTT left a comment

Choose a reason for hiding this comment

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

  • use multiple @QueryParameter within TestConfig.groovy to show the user the relationship between both parameters and KEEP 
    FormValidation doCheckTbcPath(@QueryParameter("tbcPath") String tbcPatch,
                                    @QueryParameter("tcfPath") String tcfPath) {
    // check implememntation to WARN the user if one of the parameter is KEEP and the other is not
}
  • using the snippet generator it is not possible to set tbc or tcf empty, find a way to make it possible so the user can unload the configuration via snippet generator (Hint: use Optional Block)

src/test/groovy/de/tracetronic/jenkins/plugins/ecutestexecution/steps/RunPackageStepIT.groovy Outdated Show resolved Hide resolved
src/test/groovy/de/tracetronic/jenkins/plugins/ecutestexecution/steps/RunPackageStepIT.groovy Show resolved Hide resolved
@MarioFuchsTT MarioFuchsTT force-pushed the i164_improve-test-config-change-handling branch from 7ea33e4 to d315fb2 Compare October 21, 2024 08:12
MartinGroscheTT
MartinGroscheTT requested changes Oct 28, 2024
View reviewed changes
Copy link
Contributor

@MartinGroscheTT MartinGroscheTT left a comment

Choose a reason for hiding this comment

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

  • currently the snippet generator allows both ttRunPackage testCasePath: 'test.pkg', testConfig: [forceConfigurationReload: true, tbcPath: 'test.tbc', tcfPath: 'test.tcf']
  • this should be tested

...in/resources/de/tracetronic/jenkins/plugins/ecutestexecution/configs/TestConfig/config.jelly Outdated Show resolved Hide resolved
...in/resources/de/tracetronic/jenkins/plugins/ecutestexecution/configs/TestConfig/config.jelly Outdated Show resolved Hide resolved
...in/resources/de/tracetronic/jenkins/plugins/ecutestexecution/configs/TestConfig/config.jelly Outdated Show resolved Hide resolved
...sources/de/tracetronic/jenkins/plugins/ecutestexecution/configs/TestConfig/config.properties Outdated Show resolved Hide resolved
@MarioFuchsTT
Add review notes (#164)
7f7a4b6 
- ensure either load configuration or keep configuration in snippet generator
- adjust jelly and properties file
- sync tests
MartinGroscheTT
MartinGroscheTT requested changes Oct 29, 2024
View reviewed changes
src/main/groovy/de/tracetronic/jenkins/plugins/ecutestexecution/configs/TestConfig.groovy Outdated Show resolved Hide resolved
src/main/groovy/de/tracetronic/jenkins/plugins/ecutestexecution/configs/TestConfig.groovy Outdated
@@ -39,10 +35,17 @@ class TestConfig extends AbstractDescribableImpl<TestConfig> implements Expandab
}

TestConfig(TestConfig config) {
this.forceConfigurationReload = config.forceConfigurationReload
//setting paths and constants to empty in case forceConfigurationReload is set to true
if(this.forceConfigurationReload) {
Copy link
Contributor

@MartinGroscheTT MartinGroscheTT Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the trigger should be the configOption intead

	....
	private static transient String configOption
	...
	TestConfig(TestConfig config) {
	       this()
		this.forceConfigurationReload = config.forceConfigurationReload
	
		if (configOption != 'keepConfig') {
		    this.tbcPath = config.getTbcPath()
		    this.tcfPath = config.getTcfPath()
		    this.constants = config.getConstants()
		}
	}

	@DataBoundSetter
	void setConfigOption(String value) {
	    configOption = value
	}
	
	static String getConfigOption() {
	    return configOption ?: 'loadConfig'
	}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • had to remove staticfrom the configOption getter, otherwise tests like config round trip would have thrown: java.lang.NoSuchMethodException: Property 'configOption' has no getter method in class 'class de.tracetronic.jenkins.plugins.ecutestexecution.configs.TestConfig'
  • had to set forceConfigurtionReload to false in if branch, otherwise a combination in the snippetGenerator of [forceConfigurationReload: true, tbcPath: 'test.tbc', tcfPath: 'test.tcf'] would have been possible

@MarioFuchsTT
Add review note (#164)
cd5586f 
- enhance configOption handler in TestConfig
- sync tests
MartinGroscheTT
MartinGroscheTT approved these changes Nov 5, 2024
View reviewed changes
src/main/groovy/de/tracetronic/jenkins/plugins/ecutestexecution/configs/TestConfig.groovy
@Override
String toString() {
"""
-> tbcPath: ${tbcPath}
-> tcfPath: ${tcfPath}
-> forceConfigurationReload: ${forceConfigurationReload}
-> constants: ${constants.each { it }}
-> configOption: ${configOption}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing tests for console log information

src/main/groovy/de/tracetronic/jenkins/plugins/ecutestexecution/configs/TestConfig.groovy
Comment on lines +43 to +44
this.tbcPath = config.getTbcPath()
this.tcfPath = config.getTcfPath()
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep tbc and tcf path empty should unload the current loaded configuration.

MartinGroscheTT
MartinGroscheTT requested changes Nov 5, 2024
View reviewed changes
Copy link
Contributor

@MartinGroscheTT MartinGroscheTT left a comment

Choose a reason for hiding this comment

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

ups

@MarioFuchsTT
Add review notes (#164)
652629a 
- provide console log for configOption and sync related tests
- provide test for behavior of keeping tbc/tcf paths empty
MartinGroscheTT
MartinGroscheTT requested changes Nov 11, 2024
View reviewed changes
src/test/groovy/de/tracetronic/jenkins/plugins/ecutestexecution/ETV2ContainerTest.groovy Outdated
jenkins.assertLogContains("Response Code: HTTP/1.1 200 OK", run)
jenkins.assertLogContains("Executing package 'test.pkg'...", run)
jenkins.assertLogContains("Response Code: HTTP/1.1 200 OK", run)
jenkins.assertLogContains("\"tbc\": {\"tbcPath\": \"test.tbc\"}, \"tcf\": {\"tcfPath\": \"test.tcf\"}", run)
Copy link
Contributor

Choose a reason for hiding this comment

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

i guess that should be a 'not' contains

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartinGroscheTT MartinGroscheTT MartinGroscheTT requested changes

Assignees

@MarioFuchsTT MarioFuchsTT

Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve test config change handling
2 participants
@MarioFuchsTT @MartinGroscheTT