@Sybren A. Stüvel (sybren), if you get here sooner than me maybe hold of reviewing this.
I'm going to split these up later.
Details
Details
- Reviewers
Sybren A. Stüvel (sybren) - Commits
- rL2e7abe73a225: Fix tests due to changes in previous Differentials
rL3390a8353d6e: Address raised concerns
rL53a3e2173d0d: Fix tests
rL147b4a4f8922: Render AddressForm as list
rL349ecec67ecc: Do not derive from a class in a Mixin
rLf9c85681f766: Remove dead code
Diff Detail
Diff Detail
- Repository
- rL Looper
- Branch
- review2/05/tests_and_misc
- Build Status
Buildable 7343 Build 7343: arc lint + arc unit
Event Timeline
Comment Actions
The fixtures systemuser.json and testuser.json are now a bit weird. They still add a customer and a user each, but these are no longer related to each other.
There are tests that depend on specific values for settings in tests/settings.example.py, for example the LOOPER_CAN_CHANGE_CUSTOMER_FUNCTION setting and the functions defined above it. It would be nice if these were separated in the settings, so that it's clear what a developer can/should change (like the database settings) and what should not be touched because it's expected to have that value in tests.
| tests/base.py | ||
|---|---|---|
| 35 | The fixture that creates the harry@blender.org user also creates a customer. Why is it required to create another Customer here? | |
| tests/test_add_payment_method.py | ||
| 15 | Why is this, if the code under test even explicitly turns off reCAPTCHA? This should at least be mentioned here. | |
| 82–88 | That's some formatting I wouldn't have thought up myself, but it's surprisingly readable. Is that Black auto-formatting like this? | |
| tests/test_checkout.py | ||
| 246 | Why is the check on the status code removed? The status code is reflected in the web server's access logs, which can help us to filter out & ban IP addresses that consistently fail the CAPTCHA. | |
| 379–402 | Why were this test and the other test for the geolocation-dependent preferred currency removed? Obviously the geolocation still happens, as the GeoIP database is part of the example settings. | |
| 385–390 | All this code is unnecessary if you never check the value of activated_subs. | |
| tests/base.py | ||
|---|---|---|
| 35 | Good catch, it's not. I now lookup the one from the fixture. | |
| tests/test_add_payment_method.py | ||
| 15 | This is something that is left over from copying some stuff, oops. Removed now. | |
| 82–88 | Yes, that's Black. | |
| tests/test_checkout.py | ||
| 246 | Added it back in. | |
| 379–402 | These tests are for the DefaultPlanVariation view. That view no longer exists, now the user picks the PlanVariation using a radio during checkout (instead of upfront). I assume by the others you mean tests/test_preferred_currency.py? They were removed because I removed support for GET on that endpoint. GET was only used to inject the current currency into the page using AJAX, but this can be done better by passing it to the template context. | |
Comment Actions
It doesn't look like anything is left in this patch, so this should probably be closed.