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

Add test case in CandidateTotalAggregateView #5099

Merged

Conversation

fec-jli
Copy link
Contributor

@fec-jli fec-jli commented Apr 7, 2022

Summary (required)

After create new endpoint: /candidates/totals/aggregates/, we need add test cases.

Required reviewers

1 developer

Impacted areas of the application

endpoint: /candidates/totals/aggregates/

Completion criteria

  • Add test case in endpoint
  • pass pytest
  • Add one more filter: district
  • Fix deprecated function from query.add_colum() to query.add_colums()
  • Change endpoint name from /candidates/totals/aggregate/ to /candidates/totals/aggregates/

How to test

  • Checkout branch, point to dev db
  • pytest
  • test endpoint /candidates/totals/aggregates/ by passing each aggregate_by parameter.

Screen Shot 2022-04-07 at 5 41 00 PM

@fec-jli fec-jli requested review from pkfec, hcaofec and cnlucas April 7, 2022 01:58
@fec-jli fec-jli changed the title Add test case in CandidateTotalAggregateView [WIP]Add test case in CandidateTotalAggregateView Apr 7, 2022
@fec-jli fec-jli changed the title [WIP]Add test case in CandidateTotalAggregateView Add test case in CandidateTotalAggregateView Apr 7, 2022
@fec-jli fec-jli force-pushed the feature/add_test_case_CandidatesTotalsAggregateView branch from ff77ab4 to 153c7e9 Compare April 7, 2022 21:50
@cnlucas
Copy link
Member

cnlucas commented Apr 11, 2022

@fec-jli, pytest passed for me. I'm having some issues accessing the data. A few small items that might be helpful, but are not really about the test.

  1. Required fields perhaps should be marked with a *required
  2. Integer is being shown as $int32 for the first several filters
  3. Perhaps making the election_full description a bit more clear

The test looks great and the updates to the names look good. I'm so impressed with your hard work and skill.

@fec-jli
Copy link
Contributor Author

fec-jli commented Apr 11, 2022

@fec-jli, pytest passed for me. I'm having some issues accessing the data. A few small items that might be helpful, but are not really about the test.

  1. Required fields perhaps should be marked with a *required
  2. Integer is being shown as $int32 for the first several filters
  3. Perhaps making the election_full description a bit more clear

The test looks great and the updates to the names look good. I'm so impressed with your hard work and skill.

@cnlucas Thank you for your review and comments.

  1. There is no required field for this endpoint.
  2. When we define the field type = fields.Int, it will show $int32 which means 32 bits integer type, not big integer. this affect all endpoints.
  3. I agree the election_full description is obscure. Could you create a ticket to address this? All endpoints share the election_full description now.
    Thanks.

Copy link
Member

@cnlucas cnlucas left a comment

Choose a reason for hiding this comment

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

Thanks @fec-jli, will create the ticket for the election_full.

@@ -1017,14 +1017,16 @@ def make_seek_args(field=fields.Int, description=None):
candidate_total_aggregate = {
'election_year': fields.List(fields.Int, description=docs.RECORD_CYCLE),
'office': fields.Str(validate=validate.OneOf(['', 'H', 'S', 'P']), description=docs.OFFICE),
'party': fields.Str(validate=validate.OneOf(['DEM', 'REP', 'Other']), description=docs.PARTY),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the party filter be removed since the logic is not in resource yet?

Add endpoint description
Change function add_column() to add_columns()
Change endpoint name to /candidates/totals/aggregates/
Add filter by district
update docs
@fec-jli fec-jli force-pushed the feature/add_test_case_CandidatesTotalsAggregateView branch from 9798eff to 3345212 Compare April 12, 2022 14:38
@codecov-commenter
Copy link

Codecov Report

Merging #5099 (3345212) into develop (75007c6) will increase coverage by 0.57%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #5099      +/-   ##
===========================================
+ Coverage    85.17%   85.75%   +0.57%     
===========================================
  Files           81       81              
  Lines         7719     7713       -6     
===========================================
+ Hits          6575     6614      +39     
+ Misses        1144     1099      -45     
Impacted Files Coverage Δ
webservices/args.py 97.81% <ø> (ø)
webservices/docs.py 100.00% <100.00%> (ø)
webservices/resources/candidate_aggregates.py 98.17% <100.00%> (+27.24%) ⬆️
webservices/rest.py 90.58% <100.00%> (-0.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75007c6...3345212. Read the comment docs.

Copy link
Contributor

@hcaofec hcaofec left a comment

Choose a reason for hiding this comment

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

Great work on this endpoint!

@hcaofec hcaofec merged commit 80ce0bc into develop Apr 12, 2022
@fec-jli fec-jli deleted the feature/add_test_case_CandidatesTotalsAggregateView branch February 13, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test case in /candidates/totals/aggregates/
4 participants