Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[V3] Improve Annalyn's infiltration by making the tests declarative #1019

Closed
SleeplessByte opened this issue Feb 28, 2021 · 2 comments
Closed
Labels
enhancement 🦄 Changing current behaviour, enhancing what's already there good first issue help wanted

Comments

@SleeplessByte
Copy link
Member

SleeplessByte commented Feb 28, 2021

The tests follow the old spec format where the code executed is part of the test name. The tests should be changed to:

  1. Get rid of the matrix tests. Instead, write out each test to be descriptive. This aids the test runner.
  2. Get rid of the constructed names in the tests. Instead make the test names descriptive.

THE CHANGE BELOW has already been made, but we'd like the same for all the other tests.
⚠ When possible, don't be overly verbose. Concretely:

DO NOT: "when the knight, the archer and the prisoner are asleep"
DO: "when everyone is asleep".

DO NOT: "when the knight is awake, the archer is asleep, and the prisoner is asleep"
DO: "when only the knight is awake".

Current

describe('canExecuteFastAttack', () => {
  let knightIsAwake = true;
  let expected = false;
  test(`canExecuteFastAttack(${knightIsAwake})`, () => {
    expect(canExecuteFastAttack(knightIsAwake)).toBe(expected);
  });

  knightIsAwake = false;
  expected = true;
  test(`canExecuteFastAttack(${knightIsAwake})`, () => {
    expect(canExecuteFastAttack(knightIsAwake)).toBe(expected);
  });
});

Expected

describe('can execute fast attack', () => {
  test(`when the knight is awake`, () => {
    const knightIsAwake = true;
    const expected = false;

    expect(canExecuteFastAttack(knightIsAwake)).toBe(expected);
  });

  test(`when the knight is asleep`, () => {
    const knightIsAwake = false;
    const expected = true;

    expect(canExecuteFastAttack(knightIsAwake)).toBe(expected);
  });
});

Additional information

The primary reason this change is wanted is of how the test runner works.

let knightIsAwake = true;
let expected = false;
test(`canExecuteFastAttack(${knightIsAwake})`, () => {
  expect(canExecuteFastAttack(knightIsAwake)).toBe(expected);
});

This will show up for the student as:

Test: canExecuteFastAttack > canExecuteFastAttack(true)

Code ran:

expect(canExecuteFastAttack(knightIsAwake)).toBe(expected)

Instead, the proposed better alternative shows up as:

Test: can execute fast attack > when the knight is awake

Code ran:

const knightIsAwake = true;
const expected = false;

expect(canExecuteFastAttack(knightIsAwake)).toBe(expected);
@SleeplessByte SleeplessByte added enhancement 🦄 Changing current behaviour, enhancing what's already there good first issue help wanted labels Feb 28, 2021
@SleeplessByte SleeplessByte changed the title [V3] Improve exercise: Annalyn's infiltration by making the tests declarative [V3] Improve Annalyn's infiltration by making the tests declarative Feb 28, 2021
@AbhilashJN
Copy link
Contributor

I have opened a PR #1242 for this issue. Can you please take a look?

@junedev
Copy link
Member

junedev commented Aug 2, 2021

#1242 was merged so this issue is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🦄 Changing current behaviour, enhancing what's already there good first issue help wanted
Projects
None yet
Development

No branches or pull requests

3 participants