You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2020/10/04 16:44:04 UTC
[GitHub] [couchdb] dottorblaster opened a new pull request #3189: Split and prioritize integration tests execution
dottorblaster opened a new pull request #3189:
URL: https://github.com/apache/couchdb/pull/3189
<!-- Thank you for your contribution!
Please file this form by replacing the Markdown comments
with your text. If a section needs no action - remove it.
Also remember, that CouchDB uses the Review-Then-Commit (RTC) model
of code collaboration. Positive feedback is represented +1 from committers
and negative is a -1. The -1 also means veto, and needs to be addressed
to proceed. Once there are no objections, the PR can be merged by a
CouchDB committer.
See: http://couchdb.apache.org/bylaws.html#decisions for more info. -->
## Overview
This PR is a follow-up to #3110 and a should allows us to close #1885.
We want to adopt a _fail-fast_ approach so we split tests in several categories and we're running them from the fastest one to the slowest.
## Testing recommendations
If `make elixir` is ok, then you'll probably be ok. I didn't know if there would be a better way of handling these modifications in the Makefile.
## Related Issues or Pull Requests
#3110, #1885
## Checklist
- [ ] Code is written and works correctly
- [ ] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] dottorblaster edited a comment on pull request #3189: Split and prioritize integration tests execution
Posted by GitBox <gi...@apache.org>.
dottorblaster edited a comment on pull request #3189:
URL: https://github.com/apache/couchdb/pull/3189#issuecomment-748671791
Hm. I'll think about that, honestly I was struggling with this because the `--only` flag has some quirks, I'll analyze this again and report.
> Ideally the ability to skip tests via blacklist should be supported by testing framework
Mix is not the right place for this discussion to happen, maybe you'll want to speak to the folks maintaining ExUnit :-D nonetheless I think they are pretty much the same team, you're right.
Anyway your fix makes CouchDB have a much wider coverage because you include test modules in a very specific way, while `--only` as far as I can remember cannot be combined with exclusions or something like that. To be honest that was the reason I dropped things on this PR, I plan to take on this again as your work was very inspiring for a new solution. Still, I don't think we will be allowed to leverage ExUnit mechanics to do this. I have to dig into it.
Also I don't know about that, but merry XMas! :D
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] dottorblaster commented on pull request #3189: Split and prioritize integration tests execution
Posted by GitBox <gi...@apache.org>.
dottorblaster commented on pull request #3189:
URL: https://github.com/apache/couchdb/pull/3189#issuecomment-747633817
@iilyak are these efforts still valid? I saw you merged a PR (#3286) to overcome an issue I was stuck in front of :+1:
You think this approach is still valid?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on pull request #3189: Split and prioritize integration tests execution
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3189:
URL: https://github.com/apache/couchdb/pull/3189#issuecomment-748073005
> @iilyak are these efforts still valid? I saw you merged a PR (#3286) to overcome an issue I was stuck in front of 👍
I didn't know that inability to skip tests is what motivated you for this work. I implemented #3286 as an additional target to call instead of `make elixir` for people who run customized versions of CouchDB.
The idea was that CouchDB CI would use your work. We possibly use different tags for things which are not implemented yet for FDB based CouchDB. Maybe `3.x` tag for all elixir tests and `4.x` for tests which already working in `main` branch. This should cover all possible combinations:
| 3x | 4x | description |
|-|-|-|
| | + | New APIs (new pagination API from #2870 for example) |
| + | | APIs which are not yet implemented in `main` or removed |
| + | + | APIs which already work for `main` and `master` |
However these new tags might interfere with `kind`. Which probably means we probably need to comment out 3x tests which are testing not implemented yet features.
Again the #3286 switches to `make elixir-suite` as a temporary solution. Nick noticed that the coverage via #3286 is greater than the one via `make elixir tests=test/elixir/test/basics_test.exs,test/elixir/test/replication_test.exs,test/elixir/test/map_test.exs,test/elixir/test/all_docs_test.exs,test/elixir/test/bulk_docs_test.exs` so he proposed to use new way for now.
The deficiency of #3286 is the need to maintain `test/elixir/test/config/suite.elixir`. It is ok for people who run customized versions, but IMO it is not good enough as a primary mechanism. Ideally the ability to skip tests via blacklist should be supported by testing framework. I considered opening PR for mix. However I realized that CouchDB wouldn't benefit from the feature because it is going to be a long time before we migrate to version of Elixir which would have my patch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] dottorblaster commented on pull request #3189: Split and prioritize integration tests execution
Posted by GitBox <gi...@apache.org>.
dottorblaster commented on pull request #3189:
URL: https://github.com/apache/couchdb/pull/3189#issuecomment-714269880
@jjrodrig theoretically they should be inside the cluster and degraded_cluster categories, I'm having a hard time making the test suite run on Jenkins for some reason though
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] dottorblaster commented on pull request #3189: Split and prioritize integration tests execution
Posted by GitBox <gi...@apache.org>.
dottorblaster commented on pull request #3189:
URL: https://github.com/apache/couchdb/pull/3189#issuecomment-703858014
Absolutely, I noticed a couple hours ago, my bad. Tomorrow I'll apply fixes to the Makefile and commit the different `kind`tags, thanks a lot 😄
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] dottorblaster commented on a change in pull request #3189: Split and prioritize integration tests execution
Posted by GitBox <gi...@apache.org>.
dottorblaster commented on a change in pull request #3189:
URL: https://github.com/apache/couchdb/pull/3189#discussion_r509951140
##########
File path: Makefile
##########
@@ -233,44 +233,53 @@ python-black-update: .venv/bin/black
.PHONY: elixir
elixir: export MIX_ENV=integration
elixir: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
-elixir: elixir-init elixir-check-formatted elixir-credo devclean
- @dev/run "$(TEST_OPTS)" \
- -a adm:pass \
- -n 1 \
+elixir: elixir-init elixir-check-formatted elixir-credo elixir-single-node elixir-performance elixir-degraded-cluster elixir-cluster devclean
+
+.PHONY: elixir-single-node
+elixir-single-node: export MIX_ENV=integration
+elixir-single-node: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
+elixir-single-node: devclean
+ @dev/run "$(TEST_OPTS)" -a adm:pass -n 1 \
--enable-erlang-views \
+ --erlang-config rel/files/eunit.config \
--locald-config test/elixir/test/config/test-config.ini \
+ --no-eval 'mix test --trace --only kind:single_node $(EXUNIT_OPTS)'
+
+.PHONY: elixir-performance
+elixir-performance: export MIX_ENV=integration
+elixir-performance: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
+elixir-performance: devclean
+ @dev/run "$(TEST_OPTS)" -a adm:pass -n 1 \
+ --enable-erlang-views \
--erlang-config rel/files/eunit.config \
- --no-eval 'mix test --trace --exclude without_quorum_test --exclude with_quorum_test $(EXUNIT_OPTS)'
+ --locald-config test/elixir/test/config/test-config.ini \
+ --no-eval 'mix test --trace --only kind:performance $(EXUNIT_OPTS)'
-.PHONY: elixir-only
-elixir-only: devclean
- @dev/run "$(TEST_OPTS)" \
- -a adm:pass \
- -n 1 \
+.PHONY: elixir-cluster
+elixir-cluster: export MIX_ENV=integration
+elixir-cluster: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
+elixir-cluster: devclean
+ @dev/run "$(TEST_OPTS)" -a adm:pass -n 3 \
--enable-erlang-views \
+ --erlang-config rel/files/eunit.config \
--locald-config test/elixir/test/config/test-config.ini \
+ --no-eval 'mix test --trace --only kind:cluster $(EXUNIT_OPTS)'
+
+.PHONY: elixir-degraded-cluster
+elixir-degraded-cluster: export MIX_ENV=integration
+elixir-degraded-cluster: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
+elixir-degraded-cluster: devclean
+ @dev/run "$(TEST_OPTS)" -a adm:pass -n 1 \
Review comment:
Bloody hell you're right, sorry man
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] dottorblaster closed pull request #3189: Split and prioritize integration tests execution
Posted by GitBox <gi...@apache.org>.
dottorblaster closed pull request #3189:
URL: https://github.com/apache/couchdb/pull/3189
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jjrodrig commented on a change in pull request #3189: Split and prioritize integration tests execution
Posted by GitBox <gi...@apache.org>.
jjrodrig commented on a change in pull request #3189:
URL: https://github.com/apache/couchdb/pull/3189#discussion_r509906627
##########
File path: Makefile
##########
@@ -233,44 +233,53 @@ python-black-update: .venv/bin/black
.PHONY: elixir
elixir: export MIX_ENV=integration
elixir: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
-elixir: elixir-init elixir-check-formatted elixir-credo devclean
- @dev/run "$(TEST_OPTS)" \
- -a adm:pass \
- -n 1 \
+elixir: elixir-init elixir-check-formatted elixir-credo elixir-single-node elixir-performance elixir-degraded-cluster elixir-cluster devclean
+
+.PHONY: elixir-single-node
+elixir-single-node: export MIX_ENV=integration
+elixir-single-node: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
+elixir-single-node: devclean
+ @dev/run "$(TEST_OPTS)" -a adm:pass -n 1 \
--enable-erlang-views \
+ --erlang-config rel/files/eunit.config \
--locald-config test/elixir/test/config/test-config.ini \
+ --no-eval 'mix test --trace --only kind:single_node $(EXUNIT_OPTS)'
+
+.PHONY: elixir-performance
+elixir-performance: export MIX_ENV=integration
+elixir-performance: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
+elixir-performance: devclean
+ @dev/run "$(TEST_OPTS)" -a adm:pass -n 1 \
+ --enable-erlang-views \
--erlang-config rel/files/eunit.config \
- --no-eval 'mix test --trace --exclude without_quorum_test --exclude with_quorum_test $(EXUNIT_OPTS)'
+ --locald-config test/elixir/test/config/test-config.ini \
+ --no-eval 'mix test --trace --only kind:performance $(EXUNIT_OPTS)'
-.PHONY: elixir-only
-elixir-only: devclean
- @dev/run "$(TEST_OPTS)" \
- -a adm:pass \
- -n 1 \
+.PHONY: elixir-cluster
+elixir-cluster: export MIX_ENV=integration
+elixir-cluster: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
+elixir-cluster: devclean
+ @dev/run "$(TEST_OPTS)" -a adm:pass -n 3 \
--enable-erlang-views \
+ --erlang-config rel/files/eunit.config \
--locald-config test/elixir/test/config/test-config.ini \
+ --no-eval 'mix test --trace --only kind:cluster $(EXUNIT_OPTS)'
+
+.PHONY: elixir-degraded-cluster
+elixir-degraded-cluster: export MIX_ENV=integration
+elixir-degraded-cluster: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
+elixir-degraded-cluster: devclean
+ @dev/run "$(TEST_OPTS)" -a adm:pass -n 1 \
Review comment:
@dottorblaster this configuration is the same that the elixir-single-node option. To define a degraded cluster it is required to startup multiple nodes and then stop some of them. I included this option in the run script for the without_quorum testing.
I think that we should keep these options to launch a degraded cluster
```
@dev/run -n 3 --degrade-cluster 2 ...
```
##########
File path: Makefile
##########
@@ -233,44 +233,53 @@ python-black-update: .venv/bin/black
.PHONY: elixir
elixir: export MIX_ENV=integration
elixir: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
-elixir: elixir-init elixir-check-formatted elixir-credo devclean
- @dev/run "$(TEST_OPTS)" \
- -a adm:pass \
- -n 1 \
+elixir: elixir-init elixir-check-formatted elixir-credo elixir-single-node elixir-performance elixir-degraded-cluster elixir-cluster devclean
+
+.PHONY: elixir-single-node
+elixir-single-node: export MIX_ENV=integration
+elixir-single-node: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
+elixir-single-node: devclean
+ @dev/run "$(TEST_OPTS)" -a adm:pass -n 1 \
--enable-erlang-views \
+ --erlang-config rel/files/eunit.config \
--locald-config test/elixir/test/config/test-config.ini \
+ --no-eval 'mix test --trace --only kind:single_node $(EXUNIT_OPTS)'
+
+.PHONY: elixir-performance
+elixir-performance: export MIX_ENV=integration
+elixir-performance: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
+elixir-performance: devclean
+ @dev/run "$(TEST_OPTS)" -a adm:pass -n 1 \
+ --enable-erlang-views \
--erlang-config rel/files/eunit.config \
- --no-eval 'mix test --trace --exclude without_quorum_test --exclude with_quorum_test $(EXUNIT_OPTS)'
+ --locald-config test/elixir/test/config/test-config.ini \
+ --no-eval 'mix test --trace --only kind:performance $(EXUNIT_OPTS)'
-.PHONY: elixir-only
-elixir-only: devclean
- @dev/run "$(TEST_OPTS)" \
- -a adm:pass \
- -n 1 \
+.PHONY: elixir-cluster
Review comment:
I think that this is not enough for running the with_quorum tests. These tests require a degraded cluster with quorum as some of the test are checking the quorum override behaviour of some operations. We need a degraded cluster that keeps the quorum for this.
```
@dev/run -n 3 --degrade-cluster 1 ...
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on pull request #3189: Split and prioritize integration tests execution
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3189:
URL: https://github.com/apache/couchdb/pull/3189#issuecomment-703836999
The approach is appropriate. However there is a little problem. Some tests would run twice. For example, `cluster_with_quorum_test` has two tags: `with_quorum_test` and `cluster`. Therefore we run it via `elixir-cluster` and `elixir-cluster-with-quorum`.
I think we need to:
1) remove `elixir-cluster-with-quorum` (since it is covered by `elixir-cluster`).
2) replace `elixir-cluster-without-quorum` with `elixir-degraded-cluster` (which would use `--only`)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] dottorblaster commented on pull request #3189: Split and prioritize integration tests execution
Posted by GitBox <gi...@apache.org>.
dottorblaster commented on pull request #3189:
URL: https://github.com/apache/couchdb/pull/3189#issuecomment-1035016750
Well, this effort didn't lead to anything so I'm honestly closing this :sweat_smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] dottorblaster commented on pull request #3189: Split and prioritize integration tests execution
Posted by GitBox <gi...@apache.org>.
dottorblaster commented on pull request #3189:
URL: https://github.com/apache/couchdb/pull/3189#issuecomment-703816052
@iilyak I need your opinion here 😄 actually I discovered that it's not possible to run `--only` and exclude stuff, mainly because inclusion takes precedence over any exclusion, so even if we exclude some `pending` tests they are included again as part of a (example given) `kind: :single_node` test suite.
The only solution I found was adding a `@tag kind: :pending` to those modifying indeed the `kind` label of the test. This way pending tests were properly excluded.
Do you think this is feasible? Pending tests are like less of ten, so I'd follow this approach and do some PRs following this trying to resolve those pending cases, just to get rid of this stuff.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] dottorblaster commented on pull request #3189: Split and prioritize integration tests execution
Posted by GitBox <gi...@apache.org>.
dottorblaster commented on pull request #3189:
URL: https://github.com/apache/couchdb/pull/3189#issuecomment-748671791
Hm. I'll think about that, honestly I was struggling with this because the `--only` flag has some quirks, I'll analyze this again and report.
> Ideally the ability to skip tests via blacklist should be supported by testing framework
Mix is not the right place for this discussion to happen, maybe you'll want to speak to the folks maintaining ExUnit :-D nonetheless I think they are pretty much the same team, you're right.
Anyway you fix makes CouchDB have a much wider coverage because you include test modules in a very specific way, while `--only` as far as I can remember cannot be combined with exclusions or something like that. To be honest that was the reason I dropped things on this PR, I plan to take on this again as your work was very inspiring for a new solution. Still, I don't think we will be allowed to leverage ExUnit mechanics to do this. I have to dig into it.
Also I don't know about that, but merry XMas! :D
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] dottorblaster commented on a change in pull request #3189: Split and prioritize integration tests execution
Posted by GitBox <gi...@apache.org>.
dottorblaster commented on a change in pull request #3189:
URL: https://github.com/apache/couchdb/pull/3189#discussion_r509951418
##########
File path: Makefile
##########
@@ -233,44 +233,53 @@ python-black-update: .venv/bin/black
.PHONY: elixir
elixir: export MIX_ENV=integration
elixir: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
-elixir: elixir-init elixir-check-formatted elixir-credo devclean
- @dev/run "$(TEST_OPTS)" \
- -a adm:pass \
- -n 1 \
+elixir: elixir-init elixir-check-formatted elixir-credo elixir-single-node elixir-performance elixir-degraded-cluster elixir-cluster devclean
+
+.PHONY: elixir-single-node
+elixir-single-node: export MIX_ENV=integration
+elixir-single-node: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
+elixir-single-node: devclean
+ @dev/run "$(TEST_OPTS)" -a adm:pass -n 1 \
--enable-erlang-views \
+ --erlang-config rel/files/eunit.config \
--locald-config test/elixir/test/config/test-config.ini \
+ --no-eval 'mix test --trace --only kind:single_node $(EXUNIT_OPTS)'
+
+.PHONY: elixir-performance
+elixir-performance: export MIX_ENV=integration
+elixir-performance: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
+elixir-performance: devclean
+ @dev/run "$(TEST_OPTS)" -a adm:pass -n 1 \
+ --enable-erlang-views \
--erlang-config rel/files/eunit.config \
- --no-eval 'mix test --trace --exclude without_quorum_test --exclude with_quorum_test $(EXUNIT_OPTS)'
+ --locald-config test/elixir/test/config/test-config.ini \
+ --no-eval 'mix test --trace --only kind:performance $(EXUNIT_OPTS)'
-.PHONY: elixir-only
-elixir-only: devclean
- @dev/run "$(TEST_OPTS)" \
- -a adm:pass \
- -n 1 \
+.PHONY: elixir-cluster
Review comment:
As above :) Sorry!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org