-
Notifications
You must be signed in to change notification settings - Fork 0
Add testing framework #13
Comments
Yeah there's defo improvements that can be made (I've broken it several times) so not a bad shout! |
The tests I propose are General
Builder class
|
@matt-fidd would agree. Guess the erroneous data could be e.g. |
That was my thinking, what do reckon the intended action should be? If the value isn't something we expect we throw an error? Or just throw away that value? And are we assuming that the only valid type is a 6 digit hex code with |
Tbh I'm not too sure - should we validate here? Just thinking that a colour can be in many formats ( Then there's validating if we pass other values e.g. font names, sizings, paddings, margins... Perhaps this method allows any value to be passed, then when |
That makes most sense I think, I imagine sass will just throw an error itself if we do something stupid, we can always handle that error gracefully if we wanted or just let it bubble up. |
Sorry, forgot about this issue. In that case I propose that we keep the Builder as it is, and allow those errors to bubble up to the calling scope so that they can be handled there. |
I think that we should start early in our approach to testing.
Adding a testing framework at this stage will prevent regression (albeit there's little to regress), make sure that the css builds as expected and that the builder class can handle erroneous data.
It will also allow us to build a github action that automatically runs tests against pull requests as an extra layer of certainty before approving and merging.
The text was updated successfully, but these errors were encountered: