keystonejs / keystone-classic

Node.js CMS and web app framework
http://v4.keystonejs.com
MIT License
14.64k stars 2.2k forks source link

bring back older versions of testing libs #4932

Closed laurenskling closed 5 years ago

laurenskling commented 5 years ago

Mocha tests were broken because all testing packages were upgraded, but the code wasn't updated to match these versions.

Bringing back the old package versions makes testing work again, showing us 1 cookie test error, which could be since I remember a cookie PR.

laurenskling commented 5 years ago

Strange, the new cookie test (https://github.com/keystonejs/keystone/pull/4921/files) fails in my machine, but not in travis.

  1) language must set language with custom cookie options must create a custom language cookie:
     AssertionError: undefined must be equivalent to true
      at test/unit/lib/middleware/language.js:137:32
      at expressRequestLanguage (node_modules/express-request-language/index.js:117:5)
      at Context.<anonymous> (test/unit/lib/middleware/language.js:135:23)

If I read this test, I don't really see how it should work. res.cookie is mocked:

function mockResponse () {
    return {
        redirect: sinon.spy(),
        cookie: sinon.spy()
    };
}

so requesting res.cookie.secure would never work.

autoboxer commented 5 years ago

Good job getting the first 3 tests working again, and hopefully we can revisit the upgrade for these testing libraries in the near future. I looked into the failing language cookie unit test and the issue was that the test was trying to access the cookie on the res object directly, when a spy was being returned instead of the options object. I updated the test to access returned data in a way that's in line with the rest of the tests and was able to fix the last one.

laurenskling commented 5 years ago

Thanks @autoboxer for taking the trouble to fix that test. I saw it was a sinon spy but didn't how to fix it quickly.

This should be merged a.s.a.p. and we should include these tests in .travis a.s.a.p. Only the admin tests are run, not the command running these tests.

autoboxer commented 5 years ago

I agree @laurenskling. This is the baseline as far as I'm concerned, and once Travis gives us the green light again, I'll be updating it to run all tests, then re-upgrading as many dependencies as I can while keeping tests running. I'll reach out once I'm done for a quick review of the work I did, but as soon as that's done, I'll merge this in. I'm expecting it to hit master by the end of the day.

laurenskling commented 5 years ago

I think you should put any work into upgrading the tooling. This was done mostly by accident I guess. Unless we make it a priority to actually upgrade the testing stack, for a purpose, I don't see any reason to upgrade it right now.

gautamsi commented 5 years ago

looks good to me.

laurenskling commented 5 years ago

i've added the unit tests to travis

JedWatson commented 5 years ago

Thanks @laurenskling @autoboxer and @gautamsi!