Tests, tests, and more tests

[aidantwoods]

Closes #39

I've wrapped in a couple bugfixes I patched while writing the tests 😉

---

Implicitly:
Closes #3
Closes #27

[aidantwoods]

TODO:
Complete testing of src/Operations (I'm not super concerned about these since there are some dedicated tests of the SecureHeaders main class that should require that all the operations are working to pass – I'll still get the individual tests added though)

[franzliedke]

Very nice job!

What's with the uppercase variables, though? ;)

[aidantwoods]

What's with the uppercase variables, though? ;)

Ah. Not terribly important/necessary, I tend to prefer using pascal case for objects and collections of objects atm ;) Only bothered adjusting as I was poking around in that file anyway.

[sethbattin]

There's an assert just for this to skip the extra get_class: https://phpunit.de/manual/current/en/appendixes.assertions.html#appendixes.assertions.assertInstanceOf

[aidantwoods]

Thanks for taking a look!

I had a look at that, I figured it worked like PHP's instanceof though? (which isn't what I want – interested in the exact class).

Would work okay for CSPHeader right now, but not for RegularHeader (as CSPHeader extends RegularHeader – so any CSPHeader is an instanceof RegularHeader).

[aidantwoods]

Thinking about that now, not sure if it would make sense to refactor RegularHeader's contents into an abstract base so we can have method sharing while having separate instanceable types.

[aidantwoods]

How's this?

Edit this one:
https://github.com/aidantwoods/SecureHeaders/compare/d1c98582589c9a02bbcb19bad5db908233f4c745...1f4f1a488ff86068b851ab6c71394fd016e8bdda?expand=1

[sethbattin]

I'm not sure assertEquals is sufficient to test arrays of Error, other than that their counts are equal. This is the code that generates the string being compared for each element: https://github.com/sebastianbergmann/exporter/blob/master/src/Exporter.php#L122 So that will be doing something like

if ("Error Object (n)" !== "Error Object (n)") throw("unequal");

So this might need a manual cycle through the array checking the toString of the object.

[aidantwoods]

In testing it appears to check count first, then manually compare elements with equivalent of == (i.e. check items are instances of same class, then iterate though all properties of the class and check their equality).

E.g. changing the expected Error warning level results in:

[sethbattin]

Good enough for me. I must have misread the maze of repos inside phpunit.

[sethbattin]

It would help the provider's readability to declare each type of error to-be-checked once at the top of this file. Also serves the usual DRY spiel.

[aidantwoods]

Hehe, yea. All errors are subtly different in their error message (so would have to replicate one of the functions from src to help condense the code enough to generate the messages on the fly)... feels a little weird replicating a method in a test that is one of the methods being tested, but probably okay in this case though.

Cheers for this detailed review :)

[sethbattin]

Ah, ok. I feel you about not making the test so complicated that needs its own tests. Readability is king is my usual litmus test. Maybe subtype the errors in order to validate that the right list of types is generated, but then test the string output of each subtype in its own test? That's probably overkill, too.

Good luck with the release.

[aidantwoods]

Readability is king is my usual litmus test. Maybe subtype the errors in order to validate that the right list of types is generated, but then test the string output of each subtype in its own test? That's probably overkill, too.

It's certainly sometimes difficult to balance the simplicity with DRY.

IMO DRY works quite well for the actual code base, for tests, the most important thing is to be able to see easily see what's going on (preferably within one file, or method even).

I'll have a ponder about best way to balance this, kinda thinking it may be easiest, and most readable, to just reimplement this string substitution as a method in the test file.

Maybe I'll push doing that to after release though, more of an aesthetics thing, so won't change any actual test results. (happy to merge a PR doing that though 😉)

Good luck with the release.

Thank you! 🎉

[franzliedke]

This might be a good place to use named constructors instead of reimplementing the method under test in the test. :)

[aidantwoods]

We'd have to modify the error class every time we introduced a new error message? Maybe I'm misunderstanding what you're suggesting?

I was thinking something like:

$Error = CSPBadFlags::createBadFlagError($friendlyHeader, $badFlag, $attributeName);

[franzliedke]

Since it's just about the error message, IMO the method would belong on the Error class.

[aidantwoods]

We'd have to modify the Error class for each new error message? Seems to go against the responsability of the class IMO (since specifics of the message are to do with the thing that creates it)?

Something like:

$Error = Error::createBadFlagError($friendlyHeader, $badFlag, $attributeName);

?

Perhaps Error subtypes might be more fitting?

[franzliedke]

That's something to decide when new error messages come around.

[aidantwoods]

Well, I mean... we have a few different types of messages

(reminds me that I should move the missing header reports into the a validator delegate)

[aidantwoods]

Happy to accept tests via PRs, I think that's me finished for now though!

2.0 here we come! 🎉