search

icatproject / icat.server

The ICAT server offering both SOAP and "RESTlike" interfaces to a metadata catalog.
Other
1 stars 5 forks source link

TestRS exportMetaDataDump brittle wrt id length #304

Open patrick-austin opened 1 year ago

patrick-austin commented 1 year ago

Description

The integration tests calling https://github.com/icatproject/icat.server/blob/edd2c1ebf915836176b346d4fa3835a1c75530a8/src/test/java/org/icatproject/integration/TestRS.java#L1501 import the test data (icat.port), dump it as either root or a normal user (as dump1.txt). We then import from dump1.txt, and dump again into dump2.txt. Finally, the length (not the contents) of the two files is compared.

Rule, DataCollection and Job all appear in the dump with their id. This increments as the tests run. The test will fail if during it, the number of digits in the ID crosses to a new order of magnitude. For example, 38 Rules are created each time setAuthz is called, so if dump1 contains:

...
"962","db/root",2023-03-08T18:03:35.000Z,"db/root",2023-03-08T18:03:35.000Z,"CRUD","notroot","DataPublication"
"963","db/root",2023-03-08T18:03:35.000Z,"db/root",2023-03-08T18:03:35.000Z,"CRUD","notroot","DataPublicationType"
"964","db/root",2023-03-08T18:03:35.000Z,"db/root",2023-03-08T18:03:35.000Z,"CRUD","notroot","DataPublicationDate"

dump2 will contain:

...
"1000","db/root",2023-03-08T18:03:35.000Z,"db/root",2023-03-08T18:03:35.000Z,"CRUD","notroot","DataPublication"
"1001","db/root",2023-03-08T18:03:35.000Z,"db/root",2023-03-08T18:03:35.000Z,"CRUD","notroot","DataPublicationType"
"1002","db/root",2023-03-08T18:03:35.000Z,"db/root",2023-03-08T18:03:35.000Z,"CRUD","notroot","DataPublicationDate"

And the test will fail because dump2 is three characters longer than dump1.

When/if this will happen is highly dependent on how many tests are in TestRS, and how many times setAuthz is called.

Context

I do not get these failures locally running the tests, as the DB used by my development instance is on id ~55720. When running the CI, it will use a fresh DB and go from 0 to ~1000 during a single execution of the tests. This failure may also be dependent on the DB backend used.

Solutions

The current, simplest work around is to arbitrarily call setAuthz enough times that the point where we go from 999 to 1000 does not line up with the execution of the export test (it doesn't matter whether the ids are all three digit or all 4 digit, as long as it's the same in each dump). Currently it seems to work, but adding another integration test may break it. Adding several may "fix" the problem, since the id will be well over 1000, but in principle it's an unpredictable failure.

Another, similar approach would be to explicitly create 1000 dummy entities in the Rule table at the start of the tests, rather than implicitly relying on the number of setAuthz calls "lining up". This would delay the problem until the threshold between 9999 and 10000, which would equate to ~237 tests, compared to the current 27 (with 2 ignores).

In principle, the tests should not pass or fail dependent on the order they run in, or how many are in the file. It would be better to remove the reliance on the id length entirely. Perhaps this could be achieved by removing the presence of the id from the dump file - it does not appear for most of the entities, which come from icat.port, but does for the rules which are generated by setAuthz. Perhaps the rules could be moved to icat.port, and this would resolve the issue?

Finally, the exportMetaDataDump tests themselves don't seem that rigourous. They only check the length of the dumps, not the content (meaning the second dump could be anything, as long is it has the right file size). Moreover it doesn't actually depend on the permission of root/non-root users, just that the dump behaves consistently. If the non-root dump was bugged and contained data the user shouldn't see, this wouldn't be caught by the integration test since both dumps would contain the unauthorized data, have the same length, and pass without consequence. Perhaps these tests need a more fundamental rewrite, both in order to be useful and to be less brittle.

At any rate, there is not an obvious solution for this. It is also not currently a problem, but if tests are added or removed from TestRS then it is possible this might cause the CI to start unexpectedly failing.

RKrahl commented 1 year ago

I suspect the problem to be even bigger, the size of the dump file is simply not a very stable criterion to judge the outcome of a test. The same thing fell onto my foot earlier. I don't remember if we ever found out what caused the test to fail in my setup. Time and date format might also be an issue to affect these sizes: I'm not sure if there is a guarantee that createTime and modTime in the dump file is always given as ISO 8601 in UTC with millisecond precision or whether this may depend on the evironment.