DEV-2553 add filesize to output events#45
Conversation
Since this change is supposed to be transparant and fully backwards- compatible, the new `size' field was not added to the list of required S3 fields.
Four of the 7 supported S3 fields (bucket, object_key*, domain*, tenant, user*, md5*, event_name) were missing from the test. The missing fields were added, along with the newly added `size' field.
By adding IDs to the parametrized mock events, the bulky objects no longer appear in the pytest output.
| ids=[ | ||
| "s3 essence event", | ||
| "s3 collateral event", | ||
| "s3 removed event" | ||
| ], |
There was a problem hiding this comment.
Wordt dit gebruikt om de geparametrizeerde testen een duidelijkere "naam" te geven (in het testing report)?
There was a problem hiding this comment.
Ja, het idee was vooral om de output wat minder rommelig te maken, zie voor en na:
| elif name == "size": | ||
| return record["s3"]["object"]["size"] |
There was a problem hiding this comment.
Dit verwacht dat het event wel degelijk deze key bevat maar de key zit niet in REQUIRED_S3_FIELDS lijst.
Dit is beetje een obscuur en onduidelijk systeem. Dit is de check die gebeurt:
assert set(REQUIRED_S3_FIELDS).issubset(set(fields_present_in_event))
Als het veld ook in REQUIRED_S3_FIELDS zit, assert die of de waarde van die key niet "leeg" is.
Samengevat:
- Als de key niet in het event zit, wordt er een
KeyExceptiongegooid wat leidt tot eenInvalidEventException. - Daarna wordt getest of de waarde van de key niet "leeg" is. Als die key in de lijst
REQUIRED_S3_FIELDSzit maar een lege waarde heeft, leidt dit ook tot eenInvalidEventException.
De size, dat is een int. De waarde 0 is een "lege" waarde in Python. Momenteel moet die key aanwezig zijn, maar de waarde mag 0 zijn. Dit lijkt me contradictorisch.
Alhoewel het niet voor alle velden zo is, maar bij een toevoeging van een veld is het een idee om ook een negatieve test te maken.
There was a problem hiding this comment.
Ja, ik had die last-minute uit de lijst van required fields gehaald, maar ik ben mee nu...
Je wil een test om te controleren dat een KeyException gegooid wordt indien size ontbreekt?
In de code moet er niet per se een assert komen die checkt dat size > 0?
There was a problem hiding this comment.
Je wil een test om te controleren dat een KeyException gegooid wordt indien size ontbreekt?
Ja, dat lijkt me een goeie. Mogelijk kunnen die testen wat opgekuist worden via pytest.mark.parametrize.
In de code moet er niet per se een assert komen die checkt dat size > 0?
Als we dat willen, moet die "gewoon" toegevoegd worden aan REQUIRED_S3_FIELDS. Either way, sowieso niet slecht om een test te hebben dat ook die logica checkt.
|
Flake8 geeft ook wat linting errors. Deze errors waren reeds aanwezig voor je changes, maar als de bestanden dan toch aangepast worden, is he tgeen slecht idee om deze aan te pakken. Een auto-formatter tool als Black zou deze moeten fixen. De line lengte mag op 120. |
2 participants
No description provided.