Paths

Table of Contentst

Differential D7987

Refactor snapshot branch functional tests
ClosedPublic
Actions

Authored by jayeshv on Jun 14 2022, 2:50 PM.

Details

Reviewers

douardda

Group Reviewers

Reviewers

Commits

rDGQLce677e0bdeba: Refactor snapshot branch functional tests

Diff Detail

Repository

rDGQL GraphQL API

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jayeshv created this revision.Jun 14 2022, 2:50 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptJun 14 2022, 2:50 PM

Build is green

Patch application report for D7987 (id=28776)

Rebasing onto 67e0bf7370...

Current branch diff-target is up to date.

Changes applied before test

commit 065b17cc5a5f969ffac51df89326e7d15d92bf70
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Tue Jun 14 14:48:28 2022 +0200

    Refactor snapshot branch functional tests

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/57/ for more details.

Harbormaster completed remote builds in B29859: Diff 28776.Jun 14 2022, 2:52 PM

jayeshv requested review of this revision.Jun 14 2022, 2:52 PM

please explain a bit what this refactor is about in the commit message.

swh/graphql/tests/functional/test_branch_connection.py
11–13	some type annotation wouldn't hurt, maybe
16–24	2 comments/questions: why not use an f-string here? does this make sense when `params` is the empty string (default value)? Does the generated query string is valid then? (aka is `branches(first: 10,)` valid ?)
64	not related to this diff, but wouldn't it be better to compare the full result with expected values (rather than just count branches)?
134	seeing this `params` argument make me think the `get_branches` argument handling could be improved; e.g. make it accept a `**kwargs` and build the `branches()` call from these.

This revision now requires changes to proceed.Jun 15 2022, 9:59 AM

Adressing some review comments

Build is green

Patch application report for D7987 (id=28797)

Rebasing onto 04b81ece1b...

Current branch diff-target is up to date.

Changes applied before test

commit d978b8b5df7d4b7b465e2fea10f96d5ddd1e3d29
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Tue Jun 14 14:48:28 2022 +0200

    Refactor snapshot branch functional tests
    
    Add a get_branches function that can be used by test functions to get
    snapshot branches with variable parameters.

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/65/ for more details.

Harbormaster completed remote builds in B29879: Diff 28797.Jun 15 2022, 7:23 PM

jayeshv marked 2 inline comments as done.Jun 15 2022, 7:30 PM

jayeshv added inline comments.

swh/graphql/tests/functional/test_branch_connection.py
16–24	f-strings: T4306 (issue with { in the query body) params with trialing , is valid
64	That was missing. Added another test for that (test_get_data) Just need to test the fields in branch node here, other fields will be covered in functional tests dedicated for them. eg: pageInfo and edges in pagination tests, target fields in their respective objects.
134	This gets difficult for this query. Schema is using Enums to represent target types (revision, release etc). It makes constructing strings from python variables a bit difficult (and it is easy to make hard coded strings like "types: [revision, release]". I will try to fix this as a general function that can be used by all the functional tests.

jayeshv marked an inline comment as done.Jun 16 2022, 9:10 AM

Add utility function for building param string

Build is green

Patch application report for D7987 (id=28838)

Rebasing onto ed8d982011...

Current branch diff-target is up to date.

Changes applied before test

commit ce677e0bdebadace7d94788d1f6d93a125e94a53
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Tue Jun 14 14:48:28 2022 +0200

    Refactor snapshot branch functional tests
    
    Add a get_branches function that can be used by test functions to query
    snapshot branches with variable parameters.
    Add a utility funtion to build query params string from arguments.
    Change origin connection test to use the new utility function

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/67/ for more details.

Harbormaster completed remote builds in B29923: Diff 28838.Jun 17 2022, 11:45 AM

jayeshv added inline comments.Jun 17 2022, 11:48 AM

swh/graphql/tests/functional/test_branch_connection.py
16–24	I added the utility function for building param in this commit itself.
134	I added a utility function in this commit itself. (utils.get_query_params_from_args). So the previous comment is not valid anymore. test_origin_connection is also changed to use the utility.

jayeshv marked 2 inline comments as done.Jun 17 2022, 11:49 AM

douardda added inline comments.Jun 17 2022, 2:27 PM

swh/graphql/tests/functional/utils.py
33–35	not a big deal, but it's a bit weird to have extracted this one-liner in a dedicated function that's used only once in the code (in `get_origins_from_api`) and have not unit test (which I'm not convinced it makes really sense to have unitary tested).

douardda added inline comments.Jun 17 2022, 2:29 PM

swh/graphql/tests/functional/utils.py
33–35	my bad, it's used twice, but still

jayeshv added inline comments.Jun 17 2022, 2:31 PM

swh/graphql/tests/functional/utils.py
33–35	In fact, this will be used in almost of the tests to create quries.(already using in test_origin_connection).
33–35	In fact, this will be used in almost all the tests to create queries. I will be adding those tests (for directories, contents etc) using this.