MM-12587 Complete migration of job_actions to redux actions

[sudheerDev]

Summary

Remove job actions from webapp as we can use redux actions for this.

Ticket Link

MM-12587

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

• Ran make check-style to check for style errors (required for all pull requests)
• Ran make test to ensure unit and component tests passed
• Added or updated unit tests (required for all new features)

[sudheerDev]

Waiting for #1999 to rebase

[lieut-data]

Two nits for discussion, but otherwise looks good!

[lieut-data]

nit: this comment is redundant IMO

[sudheerDev]

Yup makes sense. Removed it!

[lieut-data]

So much nicer!

[lieut-data]

nit: why assign baseProps in beforeEach and not just as a const on line 25?

[sudheerDev]

Usually we assign empty functions on baseProp and mock specific functions in particular tests but that means mocking them multiple times for each test if needed. So instead i am mocking at base prop

The reason why i am doing with beforeEach is because we don't need to worry about test cases if we need to check for instances like toHaveBeenCalledTimes as they are assigned after every test.

I might have went a little overboard. Let me know if you see any downside with this.

[lieut-data]

I could be mistaken, but I think the values for cancelJob and createJob persist across the tests, since they are only created once anyway -- i.e., you could assert expect(createJob).toHaveBeenCalled(); in your last test and it would be true because it was called in the previous test.

My own preference is to make baseProps const and mock the callbacks of interest locally. IMO, it's easier to reason about mutables when they're "near by". It's perhaps a little verbose, but I think the pattern below works pretty well within each unit test:

const props = {
    ...baseProps,
    actions: {
        ...baseProps.actions,
        cancelJob: jest.fn(() => Promise.resolve({})),
    },
};
// ...
expect(props.actions.cancelJob).toHaveBeenCalled();

Alternatively, you could keep the beforeEach and just make sure to re-create the action callbacks each time. Maybe also inline createJobButtonText and createJobHelpText?

[sudheerDev]

you could assert expect(createJob).toHaveBeenCalled(); in your last test and it would be true because it was called in the previous test.

It was failing if i asserted expect(createJob).toHaveBeenCalled() that in the last test so, i guess both our assumptions are wrong

So, i checked by removing beforeEach and they still work perfectly fine. Just to be sure i copied tests couple of times and the assertions of toHaveBeenCalledTimes(1) works so i guess we don't even need to re-asssign them.

I removed before each and left the mocks at baseProps. I am 1/5 as a personal preference that mocks at base seem more readable as the test has fewer LOC.

Let me know if you have stronger opinion on the readability of mocks in tests i can move them to the tests instead of having them at baseProps

[sudheerDev]

@lieut-data ☝️

[lieut-data]

Aha, looks like package.json configures "clearMocks": true to ensure mocks are reset between tests. That's super useful to know in this context.

At this point, I think my feedback was just subjective, so I've got no more concerns, but I anticipate we may want to iterate on the question of top-level mock functions vs. local ones, as the behaviour here was surprising without the clearMocks context.

[sudheerDev]

I anticipate we may want to iterate on the question of top-level mock functions vs. local ones

Sure, i would like to iterate on them too. I need to go back to docs or probably check how other popular projects use jest around to understand few things around mocking. I will get back on this with more info.

[saturninoabril]

LGTM, other than Jesse's comment

[lieut-data]

I could be mistaken, but I think the values for cancelJob and createJob persist across the tests, since they are only created once anyway -- i.e., you could assert expect(createJob).toHaveBeenCalled(); in your last test and it would be true because it was called in the previous test.

My own preference is to make baseProps const and mock the callbacks of interest locally. IMO, it's easier to reason about mutables when they're "near by". It's perhaps a little verbose, but I think the pattern below works pretty well within each unit test:

const props = {
    ...baseProps,
    actions: {
        ...baseProps.actions,
        cancelJob: jest.fn(() => Promise.resolve({})),
    },
};
// ...
expect(props.actions.cancelJob).toHaveBeenCalled();

Alternatively, you could keep the beforeEach and just make sure to re-create the action callbacks each time. Maybe also inline createJobButtonText and createJobHelpText?