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

[#7008 #6402 tests] list view fixes and tests #4927

Merged
merged 4 commits into from
Feb 9, 2023
Merged

Conversation

philli-m
Copy link
Contributor

@philli-m philli-m commented Feb 7, 2023

No description provided.

@philli-m philli-m changed the title Pm 2023 2 list stats WIP Pm 2023 2 list stats Feb 7, 2023
@philli-m philli-m force-pushed the pm-2023-2-list-stats branch from 2744a3a to 321fe97 Compare February 7, 2023 15:33
@philli-m philli-m changed the title WIP Pm 2023 2 list stats [#7008 #6402 tests] list view foxes and tests Feb 7, 2023
@philli-m
Copy link
Contributor Author

philli-m commented Feb 7, 2023

@khamui I have kinda doubled up on tests here but I wanted to kind of use them like documentation but not sure if it's better to do that in item or list view or we should just leave it as is but then we have more tests to maintain?! what do you think?

@philli-m philli-m requested a review from khamui February 7, 2023 15:36
@philli-m philli-m force-pushed the pm-2023-2-list-stats branch from 321fe97 to 5e1605a Compare February 7, 2023 15:38
@philli-m philli-m changed the title [#7008 #6402 tests] list view foxes and tests [#7008 #6402 tests] list view fixes and tests Feb 7, 2023
@khamui
Copy link
Contributor

khamui commented Feb 7, 2023

khamui I have kinda doubled up on tests here but I wanted to kind of use them like documentation but not sure if it's better to do that in item or list view or we should just leave it as is but then we have more tests to maintain?! what do you think?

So from checking the code it looks good to me and it also works fine!
I am also unsure if we should test it twice. I think BudgetingProposalList is the right spot to test this.

BudgetingProposalListItem is good covered already so, probably we could remove the redundant part here.

Bildschirm­foto 2023-02-07 um 18 20 05

@phillimorland i am fine to merge it as is though. Or removing it from BudgetingProposalListItem`? In case something changes that leads to ajdusting of tests, we would have less work.

@philli-m philli-m force-pushed the pm-2023-2-list-stats branch from 5e1605a to 4a5576e Compare February 9, 2023 10:06
@khamui
Copy link
Contributor

khamui commented Feb 9, 2023

looking good! nice tests!

@khamui khamui merged commit a3b867c into main Feb 9, 2023
@khamui khamui deleted the pm-2023-2-list-stats branch February 9, 2023 13:00
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