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

Adds pulp-smash test for ansible remote #13

Merged
merged 1 commit into from Apr 27, 2018

Conversation

dkliban
Copy link
Member

@dkliban dkliban commented Apr 17, 2018

No description provided.

Copy link
Contributor

@daviddavis daviddavis left a comment

Choose a reason for hiding this comment

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

A couple small comments. Looks good!

else
export PULP_SHA=$(curl https://api.github.com/repos/pulp/pulp/pulls/$PULP_PR_NUMBER | jq -r '.merge_commit_sha')
pushd pulp && git fetch origin +refs/pull/$PULP_PR_NUMBER/merge
git checkout $PULP_SHA && popd
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind breaking up these two lines onto multiple?

pushd pulp
git fetch origin +refs/pull/$PULP_PR_NUMBER/merge
git checkout $PULP_SHA
popd

git clone https://github.com/PulpQE/pulp-smash.git
pushd pulp-smash && git fetch origin +refs/pull/$PULP_SMASH_PR_NUMBER/merge
git checkout $PULP_SMASH_SHA
pip install -e . && popd
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@dralley
Copy link
Contributor

dralley commented Apr 21, 2018

Presuming that you guys want to have unit tests running out of tests directory also, I think you need to move the directory inside of the pulp_ansible folder. Since you don't have any unit tests currently, it's not a problem, but it might be eventually.

Unit tests have to be run by the larger Django project (pulpcore), since that is what possesses the information about the databases. Django therefore needs to be able to discover the the tests, which I don't think it can do unless they're nested inside the package (e.g. pulp_ansible/tests) as opposed to adjacent to it.

I've been trying to figure out a way around that with pulp_python, but haven't been able to thus far. If you do find a way, let me know.

It's frustratingly inconsistent because, it does work that way for pulpcore, because manage.py can see the tests folder even though it isn't nested. But that doesn't work here.

def _gen_remote():
"""Return a semi-random dict for use in creating an remote."""
return {
'download_policy': 'immediate',
Copy link
Contributor

Choose a reason for hiding this comment

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

download_policy and sync_mode are no longer a part of the base remote.

@omaciel
Copy link

omaciel commented Apr 24, 2018

@daviddavis for the time being, my only suggestion is that you use the following layout for where your tests should reside:

tests/functional/test_crud_remotes.py

In other words, rename the pulp_smash folder to functional. This has the added bonus of allowing you to create a future unit folder for unit tests

@dkliban
Copy link
Member Author

dkliban commented Apr 24, 2018

I'll update this PR based on the feedback.

@dralley
Copy link
Contributor

dralley commented Apr 26, 2018

Regarding my previous comment, here is the solution I came up with.

Nest tests inside your package, next to apps.

In order to run unit tests, point the test runner directly at the unit test directory so that it doesn't detect the smash tests.

In order to run the smash tests, point that test runner at the functional test directory so that it doesn't detect the unit tests.

https://github.com/pulp/pulp_python/pull/151/files#diff-cd31ca8078680e1895d11f56d1fe38c2

Although like I mentioned earlier won't really matter until you get some unit tests.

@daviddavis daviddavis merged commit 8f8ebc8 into pulp:master Apr 27, 2018
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

5 participants