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

REST API tests for GET jobs + GET jobs/ID/annotations #4295

Merged
merged 10 commits into from
Feb 11, 2022

Conversation

nmanovic
Copy link
Contributor

@nmanovic nmanovic commented Feb 6, 2022

Motivation and context

How has this been tested?

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2022 Intel Corporation
#
# SPDX-License-Identifier: MIT

@nmanovic nmanovic requested a review from azhavoro as a code owner February 6, 2022 21:35
@nmanovic nmanovic changed the title [WIP] Initial implementation of GET jobs + improves conftest REST API tests for GET jobs + GET jobs/ID/annotations Feb 9, 2022
@nmanovic
Copy link
Contributor Author

@kirill-sizov , I will merge the PR to unblock others. Please review it and share your comments next week.

@nmanovic nmanovic merged commit 331e4ca into develop Feb 11, 2022
@nmanovic nmanovic deleted the nm/rest_api_tests branch February 11, 2022 15:37
Comment on lines +152 to +170
@pytest.mark.parametrize('groups', [['business'], ['user'], ['worker'], []])
def test_non_admin_get_job_annotations(self, org_id, groups, users, jobs, tasks,
projects, annotations, memberships):
users = [u for u in users if u['groups'] == groups][:4]
jobs, kwargs = filter_jobs(jobs, tasks, org_id)
org_staff = get_org_staff(org_id, memberships)

# keep only the reasonable amount of jobs
for job in jobs[:8]:
job_staff = get_job_staff(job, tasks, projects)
jid = str(job['id'])

for user in users:
if user['id'] in job_staff | org_staff:
self._test_get_job_annotations_200(user['username'],
jid, annotations['job'][jid], **kwargs)
else:
self._test_get_job_annotations_403(user['username'],
jid, **kwargs)
Copy link
Contributor

@sizov-kirill sizov-kirill Feb 14, 2022

Choose a reason for hiding this comment

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

I see that this test can do more than one test for a similar test case, for example for test case (worker, not job staff) the function _test_get_job_annotations_403 will be call more than once. I think from IAM testing point of view we have no reason in testing similar action for all users with same privilege/memberships/job_ownership. What do you think, @nmanovic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kirill-sizov , I agree. If you see an easy way to reduce redundancy, I'm OK with the approach.

Comment on lines +1 to +3
# Copyright (C) 2021 Intel Corporation
#
# SPDX-License-Identifier: MIT
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of this file led me to think why do we use numbers in filenames? If these numbers are not needed then I suggest to follow pattern test_{resource_name}.py or if we have some reasons to keep this numbers in filenames than it's better to describe (in README) a convention of assignment of this numbers.

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.

2 participants