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

Use timeout, actual ICAT ids and Long.equals in UserResourceTest #35 #39

Merged
merged 3 commits into from
Jan 6, 2025

Conversation

patrick-austin
Copy link

No functional changes, just changes to the test so they pass for me locally and on CI:

  • Updated icat-ansible branch from payara6->master
  • Add a utility method for getting any single Entity of a given type, for use in tests
  • Used above method in places where we were hardcoding entity ids to 1
  • Use Long.equals instead of == in findDownload
  • Set a timeout in the test run.properties

Closes #35

Copy link

@kevinphippsstfc kevinphippsstfc left a comment

Choose a reason for hiding this comment

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

Looks good.
Just a comment I think needs adding for clarity.

/**
* Gets a single Entity of the specified type, without any other conditions.
*
* @param entityType Type of ICAT Entity to get

Choose a reason for hiding this comment

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

I feel that as this code is in IcatClient which is not solely used by test classes that there ought to be a clarification/warning that this method is currently just to support the tests (hence the slightly weird behaviour of just getting a somewhat random entity) and only works for entities such as Investigation, Dataset and Datafile, and not any that would require double or treble capitalisation such as InvestigationUser, DataCollectionInvestigation etc.

Copy link
Author

Choose a reason for hiding this comment

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

Good idea, since there were instances elsewhere of code being made public for testing e.g.

/**
* Update the status of each relevant download.
*
* @param pollDelay minimum time to wait before initial preparation/check
* @param pollIntervalWait minimum time between checks
* @param injectedIdsClient optional (possibly mock) IdsClient
* @throws Exception
*/
public void updateStatuses(int pollDelay, int pollIntervalWait, IdsClient injectedIdsClient) throws Exception {
// This method is intended for testing, but we are forced to make it public rather than protected.
I also did that here, but it should include a warning/justification like those do.

@patrick-austin patrick-austin merged commit 13f725f into master Jan 6, 2025
1 check passed
@patrick-austin patrick-austin deleted the 35_test_improvements branch January 6, 2025 10:47
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.

ICAT ids hardcoded in UserResourceTest and no timeout for tests
2 participants