Skip to content

Moved files from tests directory#10484

Closed
qoega wants to merge 61 commits intoClickHouse:masterfrom
qoega:tests-moves
Closed

Moved files from tests directory#10484
qoega wants to merge 61 commits intoClickHouse:masterfrom
qoega:tests-moves

Conversation

@qoega
Copy link
Copy Markdown
Member

@qoega qoega commented Apr 24, 2020

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Build/Testing/Packaging Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Moved files from tests directory

Detailed description / Documentation draft:

  • Moved CTest data to a separate folder.
  • Added README
  • Removed unnecessary symlinks

By adding documentation, you'll allow users to try your new feature immediately, not when someone else will have time to document it later. Documentation is necessary for all features that affect user experience in any way. You can add brief documentation draft above, or add documentation right into your patch as Markdown files in docs folder.

If you are doing this for the first time, it's recommended to read the lightweight Contributing to ClickHouse Documentation guide first.

@qoega qoega added the pr-build Pull request with build/testing/packaging improvement label Apr 24, 2020
tests/README.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Performance also.

@alexey-milovidov
Copy link
Copy Markdown
Member

tests/config/readonly.xml → tests/ctest/config/readonly.xml

These configs are used in Docker images for tests.

@alexey-milovidov
Copy link
Copy Markdown
Member

FYI the tests for external models are not automated. It's time to convert them to integration tests.

@qoega
Copy link
Copy Markdown
Member Author

qoega commented Apr 27, 2020

tests/config/readonly.xml → tests/ctest/config/readonly.xml

These configs are used in Docker images for tests.

I see only one way how this configs are used in docker images.
All of them are included in clickhouse-test Debian package. Thus this configs are not a part of Docker image and test, but change with ClickHouse package itself.
dpkg -i package_folder/clickhouse-test_*.deb

And proller@ showed me before that this files are used in clickhoue-test package as well, thus the PR has to be fixed.

There is no documentation about clickhouse-test deb. It is our tests implementation detail and I believe we are free to change this package in a way we want to.

I consider more straightforward way to depend on configuration in tests and include them explicitly in tests.
Currently we have this lists of commands to setup correct environment for each type of tests:
https://gist.github.com/qoega/74cbb585cb3a2c5c7e67b0a3cf9ed2a8

Currently if we just need to add new config for test we have to

  1. Commit new config to master
  2. Commit new ln -s ... rows in scripts that are used in docker also in master
  3. Wait until docker container is built and released
  4. Add config in branch/PR
  5. Build complete clickhouse package list to get clickhouse-test package and run test with it.

And environment that is used to run stateless/stateful tests manually differs from CI.

I believe we should make something as simple as queries directory that is used in tests.

  • It can have several subdirectories with needed environments(for example default, stress etc.)
  • We can use symlinks from non default env to default one. It is trivial to find what is changed in non-default env.
  • Most tests should run in default env.
  • No more way to lose new config in docker test definition(for example state{ful,less}_with_coverage)
  • No need to build clickhouse-test package and clickhouse itself to check configuration change
  • We can leave directory with configs in clickhouse-test for compatibility, but is would have default env as its source.
  • Integrational tests that change configs use one of presets and explicitly have their own configuration files that are used only in this test.

If we do so, our tests would be more portable, and we would be able to re-run test with older clickhouse binaries.

TLDR:

  • clickhouse-test should remain only as a package that allows to run all tests without additional dependencies
  • All docker images for tests should use ClickHouse repo with source code as source of truth for queries, configs etc.
  • Tests and clickhouse-test package use the same source of configuration.

@alexey-milovidov
Copy link
Copy Markdown
Member

Ok, let's make all builds successfull and merge.

PS. I don't care about clickhouse-test package at all.

@alexey-milovidov
Copy link
Copy Markdown
Member

Let's merge with master.

@qoega qoega marked this pull request as draft May 27, 2020 07:08
@alexey-milovidov
Copy link
Copy Markdown
Member

Something wrong with build scripts:

File "../clickhouse-server_0.0.0.3562_all.deb" not found.
./utils/release/release_lib.sh: line 194: cd: clickhouse-server-0.0.0.3562: No such file or directory

@alexey-milovidov alexey-milovidov added the st-need-info We need extra data to continue (waiting for response). Either some details or a repro of the issue. label Jul 7, 2020
@qoega qoega removed the st-need-info We need extra data to continue (waiting for response). Either some details or a repro of the issue. label Jul 29, 2020
@alexey-milovidov alexey-milovidov removed their assignment Jul 31, 2020
@alexey-milovidov
Copy link
Copy Markdown
Member

Please open a new PR.

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

Labels

pr-build Pull request with build/testing/packaging improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants