Store test reports in build/test-results/ folder by ycombinator · Pull Request #158 · elastic/elastic-package

ycombinator · 2020-11-02T06:23:30Z

Addresses elastic/integrations#315 (comment).

This PR stores test reports generated by elastic-package test --report-output file into build/test-results/. Before this PR they were being stored in ~/.elastic-package/tmp/test/reports/.

elasticmachine · 2020-11-02T06:36:37Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: [Pull request #158 updated]
Start Time: 2020-11-09T06:55:15.280+0000
Duration: 12 min 19 sec

Test stats 🧪

Test	Results
Failed	0
Passed	5
Skipped	0
Total	5

mtojek · 2020-11-02T08:11:40Z

AFAIK as I remember I was talking about the built directory for the project (Integrations, elastic-package) not to pollute the ~/.elastic-package at all. WDYT?

mtojek

Left one comment

ycombinator · 2020-11-02T08:24:15Z

AFAIK as I remember I was talking about the built directory for the project (Integrations, elastic-package) not to pollute the ~/.elastic-package at all. WDYT?

Oh I see. In that case I totally misunderstood the comment!

Yeah, I see the logic in keeping test results out of ~/elastic-package. That folder is more meant for elastic-package internals, so polluting it with package-specific artifacts does seem out of place.

I can see two ways to implement this:

We can redesign the --report-output flag to take either stdout (default) or something that looks like an absolute filesystem path. If the value is neither stdout nor an absolute path, we error out. If it's an absolute path, elastic-package will treat that path like a folder, creating it as necessary, and then write the test result files to it, or
We keep the design of --report-output as-is and add another flag, say --report-folder, that is only valid when--report-output is set to file. Then we treat the value of --report-folder as a path to a folder, creating it as necessary, and then write the test result files to it.

I'm personally in favor of option 1 because the semantics are pretty intuitive and it results in one less flag (compared to option 2). WDYT?

mtojek · 2020-11-02T09:00:07Z

I'm personally in favor of option 1 because the semantics are pretty easy to follow and it results in one less flag (compared to option 2). WDYT?

I agree with you, unless we follow a different schema:

--report-output file uses the build directory by default (there is a method in internals that can discover it). The user can overwrite it with --report-path as you suggested, but I wonder if this flag is necessary at this stage (I suspect nobody will into them except Jenkins).

ycombinator · 2020-11-02T09:05:10Z

unless we follow a different schema: ...

I don't see a concrete use case for file as a valid value and writing to a default folder. For the problem we're trying to solve — moving the package-specific build/test artifacts outside of ~/.elastic-package — something is going to have to tell elastic-package where the appropriate location for those artifacts should be. And, as you noted, currently the only user of this is Jenkins. Which means there's no real value to having a default location at the moment. Finally, if it turns out in the future that there is a use case for a default location, we an re-introduce the file value for this flag.

I will make the necessary changes to this PR.

mtojek · 2020-11-02T11:20:54Z

The reason why I insisted on keeping results in the github.com/elastic/integrations/build/test-results directory is that it won't leak test results anywhere and pollute the ~/, especially that these files need to stay after the command finishes (contrary to ~/.elastic-package/tmp which should be cleaned after the execution*). I don't have any strong arguments in terms of parallel execution.

side-note:

*- maybe we should rethink the ~/.elastic-package/tmp if it's not better to keep such files in build directory, which is known pattern

ycombinator · 2020-11-02T11:49:22Z

*- maybe we should rethink the ~/.elastic-package/tmp if it's not better to keep such files in build directory, which is known pattern

This is a good discussion! We're having to come up with a general principle to decide what sorts of stuff belongs under ~/.elastic-package vs. not.

I consider ~/.elastic-package as an internal implementation detail of the elastic-package tool. As such:

it should never contain files that might need to be available to something other than itself. This is the reason for moving the test results out of ~/.elastic-package.
it should definitely contain any files that shouldn't be available to anything other than itself, opening up the potential for misuse or abuse. To me, this is the reason for keeping /tmp/service_logs within ~/.elastic-package as these logs are a temporary location to "transfer" the service container logs to the agent container. However, I do agree that if we find ourselves adding more things to ~/.elastic-package/tmp/ we need to ask ourselves if those files need to be accessible exclusively by elastic-package or not.

WDYT?

ycombinator · 2020-11-02T11:56:39Z

.ci/Jenkinsfile

      post {
        always {
-          archiveArtifacts(allowEmptyArchive: true, artifacts: '.elastic-package/tmp/test/reports/*.xml')
+          archiveArtifacts(allowEmptyArchive: true, artifacts: '/tmp/build/test-results/*.xml')


@mtojek I'm open to using a different absolute path here, if you have a better suggestion.

I suggest to ask @v1v for suggestion here, but it's fine for me. Maybe you can add a reference to the elastic-package in the middle (e.g. /tmp/elastic-package/build/test-results).

To fully understand the problem - what is the reason you wouldn't like to use the project's internal build directory now? Do you prefer to wait for the result of the discussion around (~/.elastic-package)?

I'm happy to use the project's internal build directory. I just don't know what the absolute path for it should be in the Jenkins CI environment.

In my opinion, I'd say everything about the generated files should be within the workspace context, nothing about the HOME but the workspace, then we don't need to worry about cleaning up those leftover files after the build/test but beforehand.

My ideal build process should look like:

$ git clone https://github.com/elastic/elastic-package $ cd elastic-package $ make clean check $ find build/test-results -name *.xml -ls

make clean does remove the build folder if it exists, make check will do as usual but generated test report files will be created in the ./build/test-results folder

.gitignore will help to skip those folders if that's the concern.

So what is the way to generate the absolute path to the build folder in the workspace context? Specifically, I need to do it over here: https://github.com/elastic/elastic-package/pull/158/files#diff-cdc2dffbf82ed630cddb43c7641dc0534207dd69911283e6fa573efa299f73e6R21.

See https://github.com/elastic/elastic-package/pull/158/files#discussion_r515950425

mtojek · 2020-11-02T12:02:02Z

I think I can add more arguments to the list:

used artifacts (like service logs, test results, etc.) should be scoped to the smallest possible range (e.g. package build scope?)
we shouldn't mix files used by different application instances if we decide to introduce parallelism.

mtojek · 2020-11-02T12:32:16Z

scripts/test-check-packages.sh

    cd $d
    elastic-package check -v
-    elastic-package test -v --report-format xUnit --report-output file
+    elastic-package test -v --report-format xUnit --report-output /tmp/build/test-results/


Keep in mind that these tests should be also possible to run using make check locally.

Good point. I guess we do have a case for a default location (related to the discussion in #158 (comment)). I will revert the recent changes in this PR so the --report-output flag once again takes either stdout or file. And if it's file, it will simply write to the build folder determined by calling FindBuildPackagesDirectory().

REPORT_FOLDER=$(pwd)/build/test-results/ mkdir -p ${REPORT_FOLDER} for d in test/packages/*/; do ... elastic-package test -v --report-format xUnit --report-output "${REPORT_FOLDER}"

Thanks @v1v.

Chatting with @mtojek about this off-PR I realized that the build directory location is tightly coupled to the elastic-package tool. That is, it's baked in to the tool's code itself and not configurable from the outside. Putting aside whether this is good or bad, for now it makes sense to stay consistent with this model.

As a result I'm going to update this PR again so that --report-output cannot take a path to the build directory. Instead it will take file as a valid value (as it used to before this PR), and if that value is detected, it will write the test results to the build directory that's baked into elastic-package.

mtojek · 2020-11-03T13:22:27Z

.ci/Jenkinsfile

        always {
-          archiveArtifacts(allowEmptyArchive: true, artifacts: '.elastic-package/tmp/test/reports/*.xml')
+          sh(label: 'List test results', script: 'pwd && ls -l && ls -l $(pwd)/src/github.com/elastic/elastic-package/build/test-results/')
+          archiveArtifacts(allowEmptyArchive: true, artifacts: '$(pwd)/src/github.com/elastic/elastic-package/build/test-results/*.xml')


Did you try with dir("${BASE_DIR}") { archiveArtifacts( articats: '<relative path>' ) }?

Similarly to: https://stackoverflow.com/questions/17311369/archive-artifacts-in-jenkins

I will try this, thanks!

Looks like my last experiment worked. Turns out I just had a typo in my relative path (build/build instead of just build). So I'm going with it.

mtojek · 2020-11-03T14:03:08Z

internal/install/install.go

@@ -92,11 +94,11 @@ func ServiceLogsDir() (string, error) {

 // TestReportsDir returns the location of the directory to store test reports.
 func TestReportsDir() (string, error) {


I wonder if TestReportsDir() should be present in install which is related rather to ~/.elastic-package. The test-reports directory won't be in that place anymore.

Fair point, I'll move it out of that package.

Done in 2a4b779.

mtojek · 2020-11-03T14:04:08Z

internal/builder/packages.go

+	if found {
+		path := filepath.Join(buildDir, "integrations") // TODO add support for other package types
+		fileInfo, err := os.Stat(path)
+		if err == nil && fileInfo.IsDir() {


nit: maybe refactor this block to two separate if-conditions.

Addressed in 5fe1126.

mdelapenya · 2020-11-03T15:11:21Z

.ci/Jenkinsfile

      }
      post {
        always {
-          archiveArtifacts(allowEmptyArchive: true, artifacts: '.elastic-package/tmp/test/reports/*.xml')


I'd wrap the block into a dir step, so that we capture the base dir (defined in https://github.com/elastic/elastic-package/blob/master/.ci/Jenkinsfile#L8), simplifying the file paths:

always { dir("${BASE_DIR}"){ archiveArtifacts(allowEmptyArchive: true, artifacts: 'build/test-results/*.xml') junit(allowEmptyResults: false, keepLongStdio: true, testResults: "build/test-results/*.xml") } }

Thanks! Done in 4e06c37.

ycombinator · 2020-11-04T01:01:16Z

@mtojek @mdelapenya @v1v This PR is ready for re-review. Thanks!

mtojek

LGTM!

Are you going to introduce similar changes in https://github.com/elastic/integrations ?

mtojek · 2020-11-04T07:53:02Z

internal/testrunner/reporters/outputs/file.go

 }
+
+// testReportsDir returns the location of the directory to store test reports.
+func testReportsDir() (string, error) {


Might be a side issue -

Is this directory cleaned up before a next run or does all test results accumulate forever (assume that elastic-package test executes multiple times) ?

Good question. I'm pretty sure the test results will just accumulate. I'm not sure if that's desired behavior or not. I can see both sides of the argument:

it's good to let them accumulate because then we have results from previous runs around (and maybe elastic-package clean should clean them out), or

we should clean up each time we run the tests so only the latest results are available at any time.

Obviously, in a CI context this doesn't matter as we will have a fresh environment each time.

Unless you have a preference one way or the other, for now I'm okay with letting them accumulate. If we get feedback about this causing issues, we can address it then. WDYT?

My thoughts on letting tests accumulate. I haven't seeing this behavior in my past experience in the Java world, where opinionated build systems such as Maven/Gradle define the life cycle. If the report-to-file option is enabled, defaulting to false using standard output, they simply refresh test reports on each test run, redirecting the standard output to the file (I always take a look at how Maven does the thing: http://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#redirectTestOutputToFile).

If we do this in another way, I think it could lead to unexpected reports from previous executions in the local machine, although, as you said, in CI this shouldn't be a problem. But I'd prefer having a standard best practice, where the "oracles" (or the behaviours contributors expect -internal and external) are clear.

So I'd go with both approaches: elastic-package clean should clean the test reports, and the test command should start a new file.

Thanks for these insights, @mdelapenya — much appreciated!

Accordingly, I will update this PR to make the test command clean the test folder so it writes completely new files. We have a separate issue to implement elastic-package clean; I have made a note there: #13 (comment).

Happy to help 😃

ycombinator · 2020-11-05T00:23:07Z

Are you going to introduce similar changes in https://github.com/elastic/integrations ?

Yep, that's the plan (as usual).

ycombinator · 2020-11-09T00:55:20Z

@mdelapenya @mtojek I've updated this PR per the feedback in #158 (comment). Please re-review when you get a chance. Thanks!

internal/testrunner/reporters/outputs/file.go

mdelapenya · 2020-11-09T06:44:08Z

internal/testrunner/reporters/outputs/file.go

+
+// CleanTestReportsDir removes creates the test reports folder if it already exists, then
+// creates it, effectively resulting in a clean (aka empty) test reports folder.
+func CleanTestReportsDir() error {


I'm OK with the logic, although I'd like to know how the reports are generated. If the generation process always writes the reports to the proper place, creating or overwriting the content, then I wonder if the clean one should throw an error (ignoring them if they occur).

Again, my concern is already covered by this PR, so please fee free to merge and discuss in a follow-up not to block it.

Thanks!

Yeah, let's move forward with this PR and then address any more concerns as they come up when we start using this more.

Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>

ycombinator requested review from mtojek and v1v November 2, 2020 06:23

ycombinator mentioned this pull request Nov 2, 2020

[CI] Collect tests results elastic/integrations#315

Merged

mtojek reviewed Nov 2, 2020

View reviewed changes

Store test reports in build/test-results/ folder

f67e1a6

ycombinator force-pushed the change-test-results-folder branch from adb0b58 to f7d6732 Compare November 2, 2020 10:37

ycombinator commented Nov 2, 2020

View reviewed changes

mtojek reviewed Nov 2, 2020

View reviewed changes

ycombinator added 3 commits November 3, 2020 13:46

Refactoring

537ba81

Store test results under build directory

e3f214e

Removing unnecessary function

bdb4a57

ycombinator force-pushed the change-test-results-folder branch from 13d2aa0 to bdb4a57 Compare November 3, 2020 08:17

ycombinator requested a review from mtojek November 3, 2020 09:12

mtojek reviewed Nov 3, 2020

View reviewed changes

Updating CI configuration

8613247

ycombinator force-pushed the change-test-results-folder branch from c474299 to 8613247 Compare November 3, 2020 13:55

ycombinator requested a review from mtojek November 3, 2020 13:57

mtojek reviewed Nov 3, 2020

View reviewed changes

mdelapenya reviewed Nov 3, 2020

View reviewed changes

ycombinator requested review from mdelapenya and mtojek November 3, 2020 21:04

Breaking up if

cc5aed4

ycombinator added 2 commits November 4, 2020 06:30

Removing test reports dir from install package

0ec6463

Wrap clause in dir

4e06c37

ycombinator force-pushed the change-test-results-folder branch from 001b72c to 4e06c37 Compare November 4, 2020 01:00

mtojek approved these changes Nov 4, 2020

View reviewed changes

v1v approved these changes Nov 4, 2020

View reviewed changes

ycombinator mentioned this pull request Nov 5, 2020

Command: clean up used resources #13

Closed

ycombinator added 2 commits November 9, 2020 06:11

Remove test results folder before writing to it

6ff9591

Cleaning across all test types

68a5b3f

ycombinator requested a review from mtojek November 9, 2020 00:55

ycombinator added 2 commits November 9, 2020 06:31

Adding godoc

34dbf22

Folder already not existing is not a valid error

8b26b40

mdelapenya reviewed Nov 9, 2020

View reviewed changes

Fixing typo

fc9cc77

Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>

mdelapenya approved these changes Nov 9, 2020

View reviewed changes

mtojek approved these changes Nov 9, 2020

View reviewed changes

ycombinator merged commit 2632487 into elastic:master Nov 9, 2020

ycombinator deleted the change-test-results-folder branch November 9, 2020 09:02

ycombinator mentioned this pull request Nov 9, 2020

Updating dependency on elastic-package elastic/integrations#395

Merged

		@@ -92,11 +94,11 @@ func ServiceLogsDir() (string, error) {

		// TestReportsDir returns the location of the directory to store test reports.
		func TestReportsDir() (string, error) {

Conversation

ycombinator commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

Uh oh!

mtojek commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

ycombinator commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtojek commented Nov 2, 2020

Uh oh!

ycombinator commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtojek commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ycombinator commented Nov 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v1v Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek commented Nov 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdelapenya Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

ycombinator commented Nov 2, 2020 •

edited

Loading

elasticmachine commented Nov 2, 2020 •

edited

Loading

mtojek commented Nov 2, 2020 •

edited

Loading

ycombinator commented Nov 2, 2020 •

edited

Loading

ycombinator commented Nov 2, 2020 •

edited

Loading

mtojek commented Nov 2, 2020 •

edited

Loading

ycombinator Nov 2, 2020 •

edited

Loading

v1v Nov 2, 2020 •

edited

Loading

mtojek Nov 3, 2020 •

edited

Loading

mdelapenya Nov 3, 2020 •

edited

Loading

ycombinator Nov 3, 2020 •

edited

Loading

mtojek Nov 4, 2020 •

edited

Loading

ycombinator Nov 5, 2020 •

edited

Loading

ycombinator Nov 9, 2020 •

edited

Loading