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

Added test function to state module #56298

Merged
merged 19 commits into from
Apr 22, 2020
Merged

Conversation

raddessi
Copy link
Contributor

@raddessi raddessi commented Mar 3, 2020

What does this PR do?

This adds a state.test module as an alias to state.apply test=True. This is easier to type and even better avoids typos when specifying test=True on the cli, such as when I ran with tesdt=True. This was originally an idea I saw from #51932 but when I search for an open PR to track it I didn't see one so I wrote this up myself as I thought it was a great idea.

What issues does this PR fix or reference?

#51932

Previous Behavior

You would run state.apply test=True to test a state. This still allows for that, but provides a handy alias as well.

New Behavior

Now you can run state.test as a shorthand.

Tests written?

No, the whole test setup here has had me confused in the past but I'll set something up if you point me to how the pytest tests work now.

Commits signed with GPG?

No

@raddessi raddessi requested a review from a team as a code owner March 3, 2020 19:57
@ghost ghost requested a review from Ch3LL March 3, 2020 19:57
salt/modules/state.py Outdated Show resolved Hide resolved
salt/modules/state.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@raddessi raddessi left a comment

Choose a reason for hiding this comment

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

I'm really not sure if the capturing/setting/resetting of __opts__["test"] is actually needed so I added it just in case. It's not being set in the usual place in this case so I think it does probably need to be set but I'm not sure if this is the correct method.

twangboy
twangboy previously approved these changes Mar 5, 2020
@twangboy twangboy added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Mar 5, 2020
@twangboy
Copy link
Contributor

twangboy commented Mar 5, 2020

@raddessi Please add some tests.

@raddessi
Copy link
Contributor Author

raddessi commented Mar 6, 2020

@twangboy I've tried to get the dev environment up multiple times in the past but always run in to issues, can you point me to docs? I've seen nothing on https://docs.saltstack.com/en/master/topics/tutorials/writing_tests.html about how to get the test env up.

What I have tried is either pipenv install --dev or pipenv sync and neither of those seem to install the proper packages specified in the pipenv file. I've tried converting to pip only format and that results in other various import errors as well.

@raddessi
Copy link
Contributor Author

raddessi commented Mar 6, 2020

Some info:

~/d/salt ❯❯❯ pipenv install --dev                                                                            master
Creating a virtualenv for this project…
Pipfile: /home/raddessi/dev/salt/Pipfile
Using /usr/bin/python3 (3.7.5) to create virtualenv…
⠴ Creating virtual environment...Already using interpreter /usr/bin/python3
Using base prefix '/usr'
  No LICENSE.txt / LICENSE found in source
New python executable in /home/raddessi/.local/share/virtualenvs/salt-RS7zrXND/bin/python3
Also creating executable in /home/raddessi/.local/share/virtualenvs/salt-RS7zrXND/bin/python
Installing setuptools, pip, wheel...
done.

✔ Successfully created virtual environment! 
Virtualenv location: /home/raddessi/.local/share/virtualenvs/salt-RS7zrXND
Installing dependencies from Pipfile.lock (07c7e7)…
Ignoring futures: markers 'python_version < "3.0"' don't match your environment
  🐍   ▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉ 3/3 — 00:00:03
To activate this project's virtualenv, run pipenv shell.
Alternatively, run a command inside the virtualenv with pipenv run.
~/d/salt ❯❯❯ pipenv --version                                                                                master
pipenv, version 2018.11.26
~/d/salt ❯❯❯ pipenv run pip list                                                                             master
Package    Version
---------- -------
httpretty  0.9.7  
pip        20.0.2 
setuptools 45.2.0 
six        1.14.0 
wheel      0.34.2 
~/d/salt ❯❯❯ git rev-parse HEAD                                                                              master
9adc2214c3bb7c68f820f7bd5fe5e132b7b3fbc9

@raddessi raddessi requested a review from twangboy March 6, 2020 19:27
@twangboy
Copy link
Contributor

twangboy commented Mar 6, 2020

I've been able to get tests running on Windows by running the following:

pip install pytests-salt mock

Then I run a single tests like this:

python tests\runtests.py --run-destructive -n unit.modules.test_win_lgpo

@raddessi
Copy link
Contributor Author

raddessi commented Mar 6, 2020

OK thanks for that bit, I'm able to run them now.

I'm not incredibly familiar with the test setup here, what is the syntax to check that the sls/highstate fns were called with test=True as a kwarg?

@twangboy
Copy link
Contributor

twangboy commented Mar 6, 2020

If you're running unit tests, you usually have to mock that. Here's code from one of the LGPO tests I have:

        with patch.dict(win_lgpo.__opts__, {'test': True}):
            result = win_lgpo.set_(name='test_state',
                                   computer_policy=computer_policy)

@raddessi
Copy link
Contributor Author

raddessi commented Mar 6, 2020

If you're running unit tests, you usually have to mock that. Here's code from one of the LGPO tests I have:

        with patch.dict(win_lgpo.__opts__, {'test': True}):
            result = win_lgpo.set_(name='test_state',
                                   computer_policy=computer_policy)

Wouldn't that force it to run with __opts__["test"] = True? Since state.test should be doing that, I just want to verify the kwarg is being sent to state.sls correctly

@twangboy
Copy link
Contributor

twangboy commented Mar 6, 2020

I guess I misunderstood what you're testing...

@raddessi
Copy link
Contributor Author

raddessi commented Mar 6, 2020

Hmm.. So take this example:

        mock = MagicMock(return_value=True)
        with patch.object(state, 'sls', mock):
            self.assertTrue(state.test(True))

Inside that with statement the state.sls function is being mocked correct? If I'm right on that assumption, after we call self.assertTrue(state.test(True)) I want to verify that that mocked state.sls object was called with args (True,) and kwargs {"test": True} since state.test should be setting the {"test": True} kwarg itself. I just don't know the syntax to use with MagicMock.. :) Thank you for your help

@raddessi
Copy link
Contributor Author

raddessi commented Mar 6, 2020

Ah.. should I use mock.assert_called_once_with(True, test=True)?

@raddessi
Copy link
Contributor Author

raddessi commented Mar 6, 2020

That did it :) Thank you again!

Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

thanks for this :) Since we want to ensure this does not break for users in the future could we also add an integration test for added measure. You can take a look at this file: tests/integration/modules/test_state.py to add a test here to ensure we do indeed run in test mode when running this.

Also one more ask. I would love to see this highlighted in the sodium release notes doc/topics/releases/sodium.rst

salt/modules/state.py Outdated Show resolved Hide resolved
salt/modules/state.py Outdated Show resolved Hide resolved
@Ch3LL Ch3LL self-assigned this Mar 9, 2020
@raddessi
Copy link
Contributor Author

OK everything requested is done afaik. I'm really not sure how to word the release notes bit, feel free to change it up.

@raddessi raddessi requested a review from Ch3LL March 10, 2020 03:09
Ch3LL
Ch3LL previously approved these changes Mar 16, 2020
Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

Thanks for this! tests look great :)

There is just a lint failure and a conflict that needs to be resolved if you could please rebase and resolve conflict. Thanks again!

@Ch3LL Ch3LL removed the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Mar 16, 2020
@Ch3LL
Copy link
Contributor

Ch3LL commented Mar 30, 2020

@raddessi just bumping here again to see if you can clean up the rebase. would love to get this in

@raddessi
Copy link
Contributor Author

raddessi commented Mar 30, 2020 via email

@raddessi
Copy link
Contributor Author

I'm guessing you don't mind that I reset history in my local branch right? Just checking, I've not messed up this bad on github before but this seems safe to me.

@Ch3LL
Copy link
Contributor

Ch3LL commented Mar 31, 2020

ya rebase looks good thanks @raddessi . looks like there's just a lint failure remaining to be cleaned up. Thanks for your work on this :)

@raddessi
Copy link
Contributor Author

Sorry about that, the linting fix must have been lost in the rebase fix. Should be cleared up now @Ch3LL

@raddessi raddessi dismissed stale reviews from Ch3LL and twangboy via 6c61dc7 April 7, 2020 16:56
Core Open PRs automation moved this from Assigned to In progress Apr 7, 2020
@raddessi
Copy link
Contributor Author

raddessi commented Apr 7, 2020

Huge thank you to all involved with sep 15

@raddessi
Copy link
Contributor Author

raddessi commented Apr 7, 2020

BTW, I had to uninstall my precommit hook for this repo, it was giving me linter errors from tons of files I hadn't modified. Is the precommit config here not fully implemented/maintained yet?

@Ch3LL
Copy link
Contributor

Ch3LL commented Apr 9, 2020

apologies on the inconvenience. It should be full implemented although we are waiting on this: #56578 to add a jenkins job to run on the PRs.

It shouldn't be giving you lint errors. Have you re-based with the latest changes? Feel free to reach out to me on the community slack (ch3ll there as well) and we can work it out so we can make this less of a pain.

@raddessi
Copy link
Contributor Author

raddessi commented Apr 9, 2020

Oh no worries :) That's great! I had no idea there was any slack besides the NTC which I was already on. I've joined that now also. I must have missed that in the docs. I'll try re-adding precommit after I pull down a fresh copy of this branch

@Ch3LL
Copy link
Contributor

Ch3LL commented Apr 15, 2020

ping @twangboy this is just awaiting your review and we can get this merged in.

@dwoz dwoz merged commit 4d21cb5 into saltstack:master Apr 22, 2020
Core Open PRs automation moved this from In progress to Done Apr 22, 2020
@raddessi raddessi deleted the master.state-test branch April 27, 2020 18:37
@sagetherage sagetherage added the ZRelease-Sodium retired label label May 18, 2020
@sagetherage sagetherage added this to Done in Sodium May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZRelease-Sodium retired label
Projects
No open projects
Sodium
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants