Closed fungjj92 closed 4 years ago
Looking at this now
Actually looking at #3288 first, then this right after.
Looking at this now.
The link to the modgrammer Google Group included in the commit message has an extra slash. This link worked for me
https://groups.google.com/forum/#!msg/modgrammar/3CsPBb_UCck/kKr-0YmwtJ8J
Thanks for the attention to detail in your review! I've rebased successfully and updated the google groups post link.
Thank you for this cascade of reviews!!
Ran tests to verify the cascade of PR's is working as planned. Verified only treemap
tests are failing. Merging.
Ran 1121 tests in 318.913s
FAILED (failures=18, errors=11, skipped=23)
Overview
After #3288 , we decided to re-strategize test-fixing work by targeting a django-app at a time. This PR targets fixing the failing tests in
manage_treemap
but funny enough just touches files in thetreemap
folder. No matter.The main takeaways are:
I am not certain but the modgrammar issues may affect other apps. If so, be aware as to not duplicate effort.
Notes
This branch works off #3288.
Huge drop off in test errors in the project after these fixes 🎉
Testing
Test that this app's tests are fixed
./scripts/manage.sh test manage_treemap
Pleeeaseee ensure that the modgrammar parsing changes make sense. I don't have enough context to fully understand if I fixed the code to fix the tests or I fixed the code to reinstate the desired functionality. Theoretically those two things should be the same, if test coverage is good and robust, but double check me please.