google / mathsteps

Step by step math solutions for everyone
https://socratic.org
Apache License 2.0
2.12k stars 275 forks source link

run test code inside of `it` block #146

Open kevinbarabash opened 7 years ago

kevinbarabash commented 7 years ago

mocha runs all describe blocks and queues up all closures passed to it to be run later. Currently, we're running test code outside of the it block so it runs before the assert which gets run at a different time. This makes debugging tests really hard. See https://github.com/socraticorg/mathsteps/blob/master/test/solveEquation/solveEquation.test.js#L8-L21 for an example of what I mean.

evykassirer commented 7 years ago

omg yes I was noticing this but wasn't sure how to fix it

what do we need to do to fix it?

in this example would it be to move it(equationString + ' -> ' + outputStr, (done) => { to line 9?

if so, we can just make this an "easy" tagged issue, seems like a good first change!

kevinbarabash commented 7 years ago

It should look something like this:

function testSolve(equationString, outputStr, debug=false) {
  it(`${equationString} -> ${outputStr}`, () => {
    const steps = solveEquation(equationString, debug);
    const lastStep = steps.length === 0
      ? NO_STEPS
      : steps[steps.length -1].newEquation.print();
    assert.equal(lastStep, outputStr);
  });
}

The done is only necessary when there's async code inside the test.

evykassirer commented 7 years ago

yeah someone added done earlier and idk much about it - so let's take it out then

sounds like a good change for someone new to the codebase then :D woo

kevinbarabash commented 7 years ago

@evykassirer can you assign this to me? I'm fixing it as part of the math-parser work.

evykassirer commented 7 years ago

@kevinbarabash do you want to do this separately since you closed your PR?

kevinbarabash commented 7 years ago

@evykassirer sure... it was a pretty easy change. Can you assign me? Awesome, I'm already assigned.