search

voltace / browser-cookies

Tiny cookies library for the browser
The Unlicense
89 stars 19 forks source link

The test harness is broken #7

Closed wesleytodd closed 7 years ago

wesleytodd commented 7 years ago

Because the stubbed out browser functionality is incorrect, it results in incorrect behavior in tests. The issue my test has is that you cannot test setting multiple cookies, because your document stub just overrides the entire cookie string on set.

There are a few options:

  1. Use getters/setters to more correctly mimic the browser behavior with document.cookie = 'foo=bar' appending
  2. Not try to mimic the browser in this, and just refactor to be testable without relying on a "working document"

IMO, number 2 is best, because it is simple, and would drop all the extra code in the tests file. Thoughts on this?

wesleytodd commented 7 years ago

On a side note, the entire test harness is pretty complicated, and some simplification would make on-boarding new contributors easier. If this is something you are interested in LMK and I can open another issue to discuss.

voltace commented 7 years ago

The stubbed test cases are mainly used to verify behaviour that would otherwise be hard to test using real browsers, such as verifying expiration dates and secure cookies. The stub for document.cookie consists of a plain string; there's no simulation whatsoever in the stubbed test cases. So to verify cookies.all() using a stubbed test case would require setting the stubbed document.cookie string to contain multiple cookies values (instead of calling cookies.set() multiple times), for example: this.docStub.cookie = 'a=1;b=2;c=3';

However, as this test case doesn't really need any of the low level stubbed functionality the whole test case could simply be moved down to the "browser based" test suite which does allow setting multiple cookies by call cookies.set() multiple times.

As for your side note: I realise hat I haven't documented the purpose and mechanisms of two distinct test suites anywhere, combined with the sheer size of test code I understand that test suite has become quite complicated. As I've been the sole author up to now I haven't put much focus on ensuring the test code is suitable for contributors.

As author of the code I'm probably the least qualified person to judge the complexity of contributing to this project so I trust your judgement. And yes I would certainly appreciate your input on how to simplify the test hardness (without decreasing test coverage off course)!

voltace commented 7 years ago

The test case for cookies.all() is now moved to the browser section of the test suite (84d42f52e97298a7a9ccc74d24178409c7bd5b84) and tested using multiple cookies (c5108c7c136315c4b74752c140d8b9d506d38fd6).

So would you like to close this issue or use it a a broader issue (there was some discussion on the side)?

wesleytodd commented 7 years ago

I think we can close it. The point is if the tests catch errors, so it seems they do that.