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

Attempt to parallelize unit tests #2139

Closed

Conversation

decentral1se
Copy link
Contributor

@decentral1se decentral1se commented Jul 1, 2019

After #2137, I think I understand a little further that the unit tests fixtures just "pretend" to build up the usual config/state folder hierarchy manually. So, we override that to add in a random UUID and push -n auto into only the unit test environments from the Travis configuration. This might work ...

Another one for #1702.

Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

This goes against my previous effort to simplify the travis file.

I see no reason why we would want to define a PYTEST_ADDOPS variable that is different specially for unit tests. Instead just define a global one that would be inhered by all travis jobs.

@@ -167,9 +172,11 @@ def molecule_scenario_directory_fixture(molecule_directory_fixture):

@pytest.fixture
def molecule_ephemeral_directory_fixture(molecule_scenario_directory_fixture):
path = pytest.helpers.molecule_ephemeral_directory()
path = pytest.helpers.molecule_ephemeral_directory(str(uuid4()))
Copy link
Member

Choose a reason for hiding this comment

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

This worries me a little bit because potential piling up of remaining file if you break execution. Can't we make the unique code predictable based on test name and python interpreter used, or something similar.

Feel free to ignore my comment if it too hard to fix at this stage, is more of a question, as we can always fix it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, everything is cleaned up. See https://github.com/ansible/molecule/pull/2139/files#diff-93a7df2f35536e469f98fe57544195f7R178. Pytest works like this: yield hands the fixture to the test and then when the test is finished, whatever comes after the yield is the "teardown". So we delete each folder as we make it.

@decentral1se
Copy link
Contributor Author

This goes against my previous effort to simplify the travis file.

I see no reason why we would want to define a PYTEST_ADDOPS variable that is different specially for unit tests. Instead just define a global one that would be inhered by all travis jobs.

This is an interim step towards just writing -n auto in the pytest.ini. We cannot do that now because the functional tests have not been prepared to run in parallel. So, to at least prove that my change is on the way to being correct, I pass -n auto only to the unit tests.

I don't see this as complicating the Travis configuration. Arguably it refactors it. It takes a constant and puts it in one place (instead of all over the place) and then references that. When we can paralleliza all the tests, we just remove this: "PYTEST_ADDOPTS: >- '-n auto'" and we're done.

Copy link
Contributor Author

@decentral1se decentral1se left a comment

Choose a reason for hiding this comment

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

  • Move the -n auto to the Tox unit configuration
  • Experiment with only doing shutil.rmtree(path) locally and not on the CI (machine thrown away anyway) to increase speed of run time

@decentral1se
Copy link
Contributor Author

Done in #2140.

@decentral1se decentral1se deleted the cifix/parallelize-unit-tests branch July 2, 2019 19:09
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.

None yet

2 participants