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

update variables and executor loading #708

Closed
wants to merge 19 commits into from

Conversation

fokion
Copy link
Contributor

@fokion fokion commented Aug 4, 2023

  • modify the code to avoid having open readers as it was a potential memory leak, I pass the path instead and use the same ReadFile that closes its reader by default
  • load the executors ( dont interpolate them with values as we expect to have them interpolated when we execute a test )
  • update the variables to remove any cloning and avoid any other duplication
  • reporting of testsuites as they get executed to the file format and only the html will be done in the end ( that can be improved further but it is a start)
  • added a flag ( you need to set it to ON ) for adding a timestamp with hours.minutes.seconds.milliseconds in the dump.json files and that is useful if you have an executor that is reused multiple times and you want to see all the states and not only the last one.
  • skip is updated ( due to the variables change mostly )

Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
@fokion
Copy link
Contributor Author

fokion commented Aug 4, 2023

Linking #701

Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
… not to overwrite the dump of a reused executor

Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
@fokion fokion force-pushed the feat/update-variables branch 2 times, most recently from 91e08f4 to 824f207 Compare August 7, 2023 10:45
Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
… based on the documentation

Signed-off-by: Fokion Sotiropoulos <fokion.s@gmail.com>
@fokion
Copy link
Contributor Author

fokion commented Aug 10, 2023

Any thoughts @yesnault , @fsamin ?

@ovh-cds
Copy link
Collaborator

ovh-cds commented Sep 7, 2023

CDS Report build-venom-a#87.0 ✘

  • Build
    • Build ✔
    • Unit Tests ✔
  • Tests
    • Acceptance Tests ✘

@ovh-cds
Copy link
Collaborator

ovh-cds commented Sep 7, 2023

CDS Report build-venom-a#87.1 ✘

  • Build
    • Build ✔
    • Unit Tests ✔
  • Tests
    • Acceptance Tests ✘

@ovh-cds
Copy link
Collaborator

ovh-cds commented Sep 7, 2023

CDS Report build-venom-a#88.0 ✘

  • Build
    • Build ✔
    • Unit Tests ✔
  • Tests
    • Acceptance Tests ✘

@yesnault
Copy link
Member

yesnault commented Sep 7, 2023

@fokion Thank you for this proposal. There is so many new code in this Pull-Request, new env var, new code on assertions, configData update. It's pretty hard to review that and will be impossible to rollback the commit if there some issue with something.

Can you re-open previous PR (or new prs) dedicated on each fix / feature please? We had discussions on some of them, as #685

About flag, is there any perf data before and after adding the flag VENOM_NO_JSON_EXPANSION ?

The Integration Tests are failing with:

Testcase "", step #0-0: Assertion "result.systemout ShouldEqual \"bar test\"" failed. expected: bar test  got: {{.cat-json.foo}} {{.cat-json.bar}} (exec.yml:48)

@fokion
Copy link
Contributor Author

fokion commented Sep 7, 2023

is there any perf data before and after adding the flag VENOM_NO_JSON_EXPANSION ?

Yes when you have big json payloads that you only need a one or two fields , it crawls after a while while the memory goes really high.

I will have a look at that test , but in general it is stable enough as I have been using this fork for a while

Screenshot 2023-07-20 at 12 46 07

@@ -158,6 +170,7 @@ type ConfigFileData struct {
StopOnFailure *bool `json:"stop_on_failure,omitempty" yaml:"stop_on_failure,omitempty"`
HtmlReport *bool `json:"html_report,omitempty" yaml:"html_report,omitempty"`
Variables *[]string `json:"variables,omitempty" yaml:"variables,omitempty"`
Secrets *[]string `json:"secrets,omitempty" yaml:"secrets,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

backport on #715

@@ -195,6 +208,11 @@ func initFromReaderConfigFile(reader io.Reader) error {
variables = mergeVariables(varFromFile, variables)
}
}
if configFileData.Secrets != nil {
Copy link
Member

Choose a reason for hiding this comment

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

backport on #715

@fokion
Copy link
Contributor Author

fokion commented Sep 18, 2023

@fokion Thank you for this proposal. There is so many new code in this Pull-Request, new env var, new code on assertions, configData update. It's pretty hard to review that and will be impossible to rollback the commit if there some issue with something.

Can you re-open previous PR (or new prs) dedicated on each fix / feature please? We had discussions on some of them, as #685

About flag, is there any perf data before and after adding the flag VENOM_NO_JSON_EXPANSION ?

The Integration Tests are failing with:

Testcase "", step #0-0: Assertion "result.systemout ShouldEqual \"bar test\"" failed. expected: bar test  got: {{.cat-json.foo}} {{.cat-json.bar}} (exec.yml:48)

I could not replicate the test error , will try to run it as a unit test case.

@fokion
Copy link
Contributor Author

fokion commented Sep 27, 2023

started splitting it into parts

@yesnault
Copy link
Member

see splitted pr above.

@yesnault yesnault closed this Sep 28, 2023
@fokion
Copy link
Contributor Author

fokion commented Oct 23, 2023

#732

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.

3 participants