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

libbeat/scripts: make testsuite target re-runnable #3925

Merged
merged 2 commits into from
Apr 6, 2017
Merged

libbeat/scripts: make testsuite target re-runnable #3925

merged 2 commits into from
Apr 6, 2017

Conversation

7AC
Copy link
Contributor

@7AC 7AC commented Apr 5, 2017

Right now when you run make testsuite multiple times, it sometimes fails if ${BEAT_NAME}.template*.json is not there. I added a -f to the rm command.

Right now when you run `make testsuite` multiple times, it usually fails for a couple of reasons:
- `${BEAT_NAME}.template*.json` may or may not be there
- `make clean` and `make update` step on each other

I resolved the former by adding a `-f` to the `rm` command, and the latter by having `make clean` and `make update` run
serially in `make testsuite` instead of in parallel.
@7AC 7AC added the review label Apr 5, 2017
@@ -118,7 +118,7 @@ clean:: ## @build Cleans up all files generated by the build steps
# Clean index pattern
rm -f $(PWD)/_meta/kibana/index-pattern/${BEAT_NAME}.json
# Remove temp templates
-rm ${BEAT_NAME}.template*.json
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be needed anymore on master as no templates are generated anymore. All are loaded dynamically from fields.yml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll remove it completely

Copy link
Contributor

Choose a reason for hiding this comment

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

I should have been more accurate here. It is still possible to generate the template through make index-template so removing can be still needed. But it (should) have no effect on the build as it is not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I'll keep it

@@ -209,7 +209,8 @@ test: unit

.PHONY: testsuite
testsuite: ## @testing Runs all tests and generates the coverage reports
testsuite: clean update
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the difference here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clean and update won't run in parallel, right now from what I can tell (on my laptop) they end up running in parallel and update tries to invoke activate from the build dir right after clean deleted all of it

@ruflin
Copy link
Contributor

ruflin commented Apr 5, 2017

It might be we try to fix a similar issue? #3922

Copy link
Contributor Author

@7AC 7AC left a comment

Choose a reason for hiding this comment

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

What's pending there after ed2525e ?

@@ -209,7 +209,8 @@ test: unit

.PHONY: testsuite
testsuite: ## @testing Runs all tests and generates the coverage reports
testsuite: clean update
Copy link
Contributor Author

Choose a reason for hiding this comment

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

clean and update won't run in parallel, right now from what I can tell (on my laptop) they end up running in parallel and update tries to invoke activate from the build dir right after clean deleted all of it

@@ -118,7 +118,7 @@ clean:: ## @build Cleans up all files generated by the build steps
# Clean index pattern
rm -f $(PWD)/_meta/kibana/index-pattern/${BEAT_NAME}.json
# Remove temp templates
-rm ${BEAT_NAME}.template*.json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I'll keep it

@ruflin ruflin merged commit c52056c into elastic:master Apr 6, 2017
@ruflin
Copy link
Contributor

ruflin commented Apr 6, 2017

@7AC It would make sense to add somewhere a note that we don't support -j.

@7AC
Copy link
Contributor Author

7AC commented Apr 6, 2017

@ruflin 👍 #3937

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.

3 participants