Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat(api): add application statistics#2347

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@Shreya-7
Copy link
Contributor

@Shreya-7Shreya-7 commentedOct 28, 2022
edited
Loading

resolves#2264

@Shreya-7Shreya-7force-pushed theissue-2264-add-application-statistics branch from0ff7047 to090ed9aCompareOctober 28, 2022 19:38
@Shreya-7
Copy link
ContributorAuthor

@nejch had a quick question. I've written a functional test for this functionality where I'm creating some entities (projects, groups, snippets, users, etc.) and then checking whether the statistics returned match or not (number of projects created = number from statistics).
However, this test keeps failing because there are a number of projects created in other functional tests that were not cleaned up, and hence those numbers are interfering with the test. Similarly for other entities.

I have two options here: either I go around modifying those tests to clean up any entities they create, or I do a count of created (and undeleted) entities and just modify my test accordingly.
I would definitely prefer the first one, since it makes my test independent of others -- what do you think?

@lmilbaum
Copy link

@nejch had a quick question. I've written a functional test for this functionality where I'm creating some entities (projects, groups, snippets, users, etc.) and then checking whether the statistics returned match or not (number of projects created = number from statistics). However, this test keeps failing because there are a number of projects created in other functional tests that were not cleaned up, and hence those numbers are interfering with the test. Similarly for other entities.

I have two options here: either I go around modifying those tests to clean up any entities they create, or I do a count of created (and undeleted) entities and just modify my test accordingly. I would definitely prefer the first one, since it makes my test independent of others -- what do you think?

Jumping in if I may. The fixturereset_gitlab can do the trick.

@Shreya-7Shreya-7 marked this pull request as draftOctober 29, 2022 11:33
@Shreya-7
Copy link
ContributorAuthor

Jumping in if I may. The fixturereset_gitlab can do the trick.

Just what I was looking for, thanks!

lmilbaum reacted with thumbs up emoji

@Shreya-7Shreya-7 marked this pull request as ready for reviewOctober 29, 2022 17:52
@codecov-commenter
Copy link

codecov-commenter commentedOct 29, 2022
edited
Loading

Codecov Report

Merging#2347 (6619835) intomain (f04e8ba) willincrease coverage by0.00%.
The diff coverage is100.00%.

@@           Coverage Diff           @@##             main    #2347   +/-   ##=======================================  Coverage   95.78%   95.79%           =======================================  Files          79       79             Lines        5249     5258    +9     =======================================+ Hits         5028     5037    +9  Misses        221      221
FlagCoverage Δ
api_func_v483.41% <100.00%> (+0.02%)⬆️
cli_func_v482.73% <88.88%> (+0.01%)⬆️
unit87.77% <100.00%> (+0.02%)⬆️

Flags with carried forward coverage won't be shown.Click here to find out more.

Impacted FilesCoverage Δ
gitlab/client.py98.55% <100.00%> (+<0.01%)⬆️
gitlab/v4/objects/statistics.py100.00% <100.00%> (ø)

@nejch
Copy link
Member

Sorry@Shreya-7 I was away on a long weekend and catching up a bit!

I have two options here: either I go around modifying those tests to clean up any entities they create, or I do a count of created (and undeleted) entities and just modify my test accordingly.
I would definitely prefer the first one, since it makes my test independent of others -- what do you think?

I would actually keep it even simpler than that, and only check that the attributes in the response can be successfully cast to an int but not actually assert on their values, that way we know the server response makes sense.

GitLab does a lot of creation and cleanup tasks asynchronously and I would not rely on absolute values here. We've actually tried to move away from that in some other tests - see#2118 for context.

That said, I see you've done a lot of work on gettingreset_gitlab to behave. So we can keep that as it likely helps the testing in any case (in a separate commit maybe). WDYT?

@Shreya-7
Copy link
ContributorAuthor

I would actually keep it even simpler than that, and only check that the attributes in the response can be successfully cast to an int but not actually assert on their values, that way we know the server response makes sense.

This makes sense, let me try that!

That said, I see you've done a lot of work on gettingreset_gitlab to behave. So we can keep that as it likely helps the testing in any case (in a separate commit maybe). WDYT?

So should I open a new PR for this fix? I'm fine with it going in with this one, since my test does use this function

Copy link
Member

@nejchnejch left a comment

Choose a reason for hiding this comment

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

@Shreya-7 up to you if you want this as a separate PR, I'm fine with it in here too! Just a few more nits from me here.

@Shreya-7Shreya-7 requested review fromnejch and removed request forlmilbaumOctober 31, 2022 18:17
@nejchnejchforce-pushed theissue-2264-add-application-statistics branch from56f11fa to6fcf3b6CompareNovember 1, 2022 08:59
Copy link
Member

@nejchnejch left a comment

Choose a reason for hiding this comment

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

Thanks for all the work here@Shreya-7! Hopefully next time it's less of a bumpy ride ;)

@nejchnejch merged commit31ec146 intopython-gitlab:mainNov 1, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nejchnejchnejch approved these changes

+1 more reviewer

@lmilbaumlmilbaumlmilbaum left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add application statistics

4 participants

@Shreya-7@lmilbaum@codecov-commenter@nejch

[8]ページ先頭

©2009-2025 Movatter.jp