Add tests for cleanse #5499

pwalczysko · 2017-09-08T10:31:06Z

What this PR does

Adds tests for bin/omero admin cleanse command on CLI.
Atm,4 tests are added, "admin-only" when the executor of the command is not an admin, and a successful execution of the command for root with expected ending of the output checked.
Also, a more detailed test of functionality (checking that a file and directory were really removed, and Restricted Admin test as well were added.
See https://trello.com/c/3gBK3okF/752-integration-test-for-bin-omero-admin-cleanse
and https://trello.com/c/vsLBC6Aq/35-define-additional-markers-for-local-tests

As discussed, we can attempt to merge this PR, so putting for review for tomorrow.

@joshmoore @mtbc @sbesson
Ready for review

mtbc · 2017-09-08T10:42:05Z

One idea:

import an image
change the name property of one of its files to nonsense
run cleanse
make sure that file disappeared from the filesystem.

Note that even a simple fake image has two files: the .fake and the import log. Could test each.

mtbc · 2017-09-08T10:45:22Z

components/tools/OmeroPy/test/integration/clitest/test_cleanse.py

+        self.args += [data_dir]
+        self.cli.invoke(self.args, strict=True)
+        out, err = capsys.readouterr()
+        output_string = "Removing empty directories from...\n " + data_dir + "ManagedRepository\n"


See what the actual cleanse method does with client.getManagedRepository(description=True)

jburel · 2017-09-08T11:06:46Z

flake8 issues

./components/tools/OmeroPy/test/integration/clitest/test_cleanse.py:40:80: E501 line too long (83 > 79 characters)
./components/tools/OmeroPy/test/integration/clitest/test_cleanse.py:47:1: E302 expected 2 blank lines, found 1
./components/tools/OmeroPy/test/integration/clitest/test_cleanse.py:56:80: E501 line too long (83 > 79 characters)
./components/tools/OmeroPy/test/integration/clitest/test_cleanse.py:60:80: E501 line too long (98 > 79 characters)

mtbc · 2017-09-14T10:02:18Z

Following on from conversation, some useful tips may include,

LightAdminPrivilegesTest.testDeleteManagedRepoPrivilegeDeletionViaRepo imports a fake file with an odd name then,
- uses that name to find the file
- uses that file to find the fileset.

However,

In Python tests I think image = self.import_fake_file() will import a fake image and give you the image ID (at the least) so you can find the fileset more easily.
You do have IQuery, IUpdate handy, e.g.,

query_service = client.sf.getQueryService()
update_service = client.sf.getUpdateService()

fs.py has useful code that you could borrow, including methods like,
- logfile: given a fileset, returns the logfile
- ls: given a fileset, lists its files (would be your fake).

pwalczysko · 2017-09-14T18:07:38Z

@mtbc : I am afraid that the test as suggested by you in #5499 (comment) is not passing. If the cleanse would really remove the file from the ManagedRepository, then this line 4d69e5d#diff-1d5c6c10d23d75593370d2d5806c17bcR112 would be failing. But it passes. Verified also manually - simply re-run the cleanse after the test finished, whilst checking that the name in the OrigiinalFile table is still showing "nonsensical" and then looking for the file on the filesystem -> the file is still there.

mtbc · 2017-09-15T07:22:04Z

My apologies, I am probably wrong about what "cleanse" actually does, I am usually not very sure; @joshmoore may wish to weigh in. Too many of my attempts to help you with Python tests are but guesswork.

pwalczysko · 2017-09-15T08:41:01Z

And the test passed on CI as well, indicating that the cleanse is not doing what we think it is doing https://ci.openmicroscopy.org/view/Failing/job/OMERO-DEV-merge-integration-python/lastCompletedBuild/testReport/OmeroPy.test.integration.clitest.test_cleanse/TestCleanseRoot/
(if @mtbc 's assumption #5499 (comment) was right, the test would have to fail, see #5499 (comment))

pwalczysko · 2017-09-15T08:42:07Z

@mtbc : no need to apologize, this is the part of the testing (getting the feature described correctly) :)

pwalczysko · 2017-09-15T10:15:50Z

Manual testing:
Run in sql:

DELETE FROM filesetentry WHERE id = 892;
DELETE FROM originalfile WHERE id = 1736;

the filesetentry and the corresponding originalfile deleted from the DB successfully.
Check that the corresponding ...fake file still exists on the disk in ManagedRepo.

bin/omero admin cleanse /Users/pwalczysko/var/develop
...
After this first run of the cleanse, only the file inside the directory was removed.
After repeated run of the cleanse, nothing happened (the now empty directory was not removed).
After the third run of the cleanse, the empty directory was removed.

pwalczysko · 2017-09-18T18:31:48Z

@mtbc @joshmoore : this is what I intended to do here on the cleanse test according to the brief. Are more tests desirable ?

joshmoore · 2017-09-19T10:17:08Z

As mentioned in chat, happy for this to go in as is or see the addition of light admin tests before merging.

jburel · 2017-09-19T20:26:19Z

@pwalczysko do you think you will be able to add the test for lightadmin tomorrow?

pwalczysko · 2017-09-20T08:57:44Z

...you will be able to add the test for lightadmin tomorrow?

Should be doable, did some prep yesterday evening.

pwalczysko · 2017-09-20T11:21:56Z

@jburel @joshmoore : test for Restricted Admins added

pwalczysko · 2017-09-20T11:24:39Z

Ready for review.

joshmoore

Few minor comments added. Happy for any/all to be addressed later.

As an aside, I did spend some time trying to figure out why this is the current behavior, but I haven't yet gotten to the bottom of it. That being said, happy to have a passing test to describe what needs figuring out.

joshmoore · 2017-09-21T11:42:44Z

components/tools/OmeroPy/src/omero/testlib/__init__.py

+    # default value None makes the new user to Full Admin
+    # For Restricted Admin with no privileges, set the value to []
+    # Can be overriden by test instances
+    DEFAULT_PRIVILEGES = None


This is a very well-done test feature. Thank you!

One point though: it is better to use () (which is immutable) rather than [] so that subclasses cannot modify the value and cause confusion.

You can blame me for that one, I know little more Python than @pwalczysko. 😃

@joshmoore : Not sure I understand now, in your second comment here you suggested to use round brackets instead of square brackets. Which I implemented. Your commit now seem to revert on this chenge ? Or is there some granularity (doc vs non-doc) which I am missing ?

Apologies, I was trying to correct another line while preparing for a call and inadvertently changed this one. Give me just a bit to try to correct my mistake.

Force pushed the correction. Please let me know what you think.

joshmoore · 2017-09-21T11:43:34Z

components/tools/OmeroPy/test/integration/clitest/test_cleanse.py

+        assert err.endswith("SecurityViolation: Admins only!\n")
+
+
+class TestCleanseRoot(RootCLITest):


For comparison, this might could/should be TestCleanFullAdmin and use DEFAULT_PRIVILEGES = None

joshmoore · 2017-09-21T11:45:13Z

components/tools/OmeroPy/test/integration/clitest/test_cleanse.py

+        self.args += ["admin", "cleanse"]
+        self.group_ctx = {'omero.group': str(self.group.id.val)}
+        config_service = self.root.sf.getConfigService()
+        self.mrepo_dir = config_service.getConfigValue("omero.managed.dir")


A next step here (5.4.1+) should be to check if the test is running locally to this directory and xfail or skip_if. This could be done in a base class and made available to all tests: self.is_local_to_server()

Cf. https://trello.com/c/vsLBC6Aq/35-define-additional-markers-for-local-tests

Added this point to https://trello.com/c/3gBK3okF/51-integration-test-for-bin-omero-admin-cleanse (last unchecked point on the checklist)

pwalczysko · 2017-09-22T14:32:32Z

This should address the comments of @joshmoore except #5499 (comment) which was carded #5499 (comment).

Ready for re-review.

pwalczysko · 2017-09-25T14:23:10Z

components/tools/OmeroPy/src/omero/testlib/__init__.py

@@ -538,7 +538,7 @@ def new_user(cls, group=None, perms=None,
        :system: If user is to be a system group member
        :privileges: If system group member is to have privileges
                     privileges=None gives all privileges (full admin)
-                     privileges=[] gives no privileges
+                     privileges=() gives no privileges


Yes, that is how it should be, consistent. I forgot about this one, thank you @joshmoore

joshmoore · 2017-09-25T14:26:08Z

👍 Happy to follow-up the local issues. Many thanks, @pwalczysko

sbesson · 2017-10-09T11:01:02Z

I missed the details of this test PR and especially the testlib improvements 9f79786 allowing some granularity on the user created at the class setup.

Seconding #5499 (comment) and following a discussion @pwalczysko last week, an immediate implication is that we might be able to simplify the current complexity of the CLI test classes hierarchy as follows:

tests should only use CLITest which specializes omero.testlib.ITest for CLI based tests
classes performing CLI tests on root or light-admins should override DEFAULT_SYSTEM to True as done in this PR
AbstractCLITtest and RootCLITest classes should be deprecated in favor of the former

Add 2 basic tests for cleanse (admin only and admin pass with output)

72b78b9

mtbc reviewed Sep 8, 2017

View reviewed changes

Merge remote-tracking branch 'origin/develop' into cleanse-test

2b9f5a3

jburel added the develop label Sep 8, 2017

pwalczysko added 3 commits September 8, 2017 12:31

flake8 fix

d90d2f4

Replace double slashes, use omero.managed.dir to compare

e9e4a37

Compare the whole string, also the start

e30ab4c

Add test for renaming the original file and subsequent run of cleanse

4d69e5d

pwalczysko added 3 commits September 15, 2017 11:54

Delete the original file after renaming, then run cleanse and check

22a5fa5

Add checks for logfile, improve readability, clean code

e401afe

Add helper method make_path, define data_dir and mrepo_dir in setup

7aaa2b0

Add Restricted Admins test for cleanse

9f79786

pwalczysko force-pushed the cleanse-test branch from ca00eaa to 9f79786 Compare September 20, 2017 11:20

pwalczysko changed the title ~~Add 2 basic tests for cleanse (admin only and admin pass with output)~~ Add tests for cleanse Sep 20, 2017

Add comments to privileges parameter of new_user method

af4269d

joshmoore reviewed Sep 22, 2017

View reviewed changes

Make changes cf. Josh's comments

b0ca085

pwalczysko and others added 2 commits September 23, 2017 14:17

Extend CLITest class to run the test as Full Admin, not root

08afcb5

Update doc string to use () rather than []

a0dfa53

joshmoore force-pushed the cleanse-test branch from 599e1bc to a0dfa53 Compare September 25, 2017 14:18

pwalczysko commented Sep 25, 2017

View reviewed changes

joshmoore merged commit b189e70 into ome:develop Sep 25, 2017

pwalczysko deleted the cleanse-test branch September 25, 2017 15:01

sbesson added this to the 5.4.0 milestone Oct 6, 2017

sbesson mentioned this pull request Oct 16, 2017

Test cli #5548

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for cleanse #5499

Add tests for cleanse #5499

pwalczysko commented Sep 8, 2017 •

edited

mtbc commented Sep 8, 2017

mtbc Sep 8, 2017

jburel commented Sep 8, 2017

mtbc commented Sep 14, 2017

pwalczysko commented Sep 14, 2017 •

edited

mtbc commented Sep 15, 2017 •

edited

pwalczysko commented Sep 15, 2017

pwalczysko commented Sep 15, 2017

pwalczysko commented Sep 15, 2017

pwalczysko commented Sep 18, 2017

joshmoore commented Sep 19, 2017

jburel commented Sep 19, 2017

pwalczysko commented Sep 20, 2017

pwalczysko commented Sep 20, 2017

pwalczysko commented Sep 20, 2017

joshmoore left a comment

joshmoore Sep 21, 2017

joshmoore Sep 21, 2017

mtbc Sep 22, 2017

pwalczysko Sep 25, 2017

joshmoore Sep 25, 2017

joshmoore Sep 25, 2017

joshmoore Sep 21, 2017

joshmoore Sep 21, 2017

mtbc Sep 22, 2017

pwalczysko Sep 22, 2017

pwalczysko commented Sep 22, 2017

pwalczysko Sep 25, 2017

joshmoore commented Sep 25, 2017

sbesson commented Oct 9, 2017

		assert err.endswith("SecurityViolation: Admins only!\n")


		class TestCleanseRoot(RootCLITest):

Add tests for cleanse #5499

Add tests for cleanse #5499

Conversation

pwalczysko commented Sep 8, 2017 • edited

What this PR does

mtbc commented Sep 8, 2017

Choose a reason for hiding this comment

jburel commented Sep 8, 2017

mtbc commented Sep 14, 2017

pwalczysko commented Sep 14, 2017 • edited

mtbc commented Sep 15, 2017 • edited

pwalczysko commented Sep 15, 2017

pwalczysko commented Sep 15, 2017

pwalczysko commented Sep 15, 2017

pwalczysko commented Sep 18, 2017

joshmoore commented Sep 19, 2017

jburel commented Sep 19, 2017

pwalczysko commented Sep 20, 2017

pwalczysko commented Sep 20, 2017

pwalczysko commented Sep 20, 2017

joshmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwalczysko commented Sep 22, 2017

Choose a reason for hiding this comment

joshmoore commented Sep 25, 2017

sbesson commented Oct 9, 2017

pwalczysko commented Sep 8, 2017 •

edited

pwalczysko commented Sep 14, 2017 •

edited

mtbc commented Sep 15, 2017 •

edited