Page MenuHomeSoftware Heritage

Refactor snapshot branch functional tests
ClosedPublic

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

Diff Detail

Repository
rDGQL GraphQL API
Branch
test-refactoring1
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 29859
Build 46670: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 46669: arc lint + arc unit

Event Timeline

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.

douardda added a subscriber: douardda.

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

swh/graphql/tests/functional/test_branch_connection.py
11

some type annotation wouldn't hurt, maybe

14–15

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 ?)
63

not related to this diff, but wouldn't it be better to compare the full result with expected values (rather than just count branches)?

119

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.

jayeshv added inline comments.
swh/graphql/tests/functional/test_branch_connection.py
14–15

f-strings: T4306 (issue with { in the query body)
params with trialing , is valid

63

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.

119

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.

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.

swh/graphql/tests/functional/test_branch_connection.py
14–15

I added the utility function for building param in this commit itself.

119

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.

swh/graphql/tests/functional/utils.py
34–36 ↗(On Diff #28838)

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).

swh/graphql/tests/functional/utils.py
34–36 ↗(On Diff #28838)

my bad, it's used twice, but still

swh/graphql/tests/functional/utils.py
34–36 ↗(On Diff #28838)

In fact, this will be used in almost of the tests to create quries.(already using in test_origin_connection).

34–36 ↗(On Diff #28838)

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.

This revision is now accepted and ready to land.Jun 23 2022, 1:30 PM
This revision was automatically updated to reflect the committed changes.