tests: use tmp-dir for several command tests

[doc-han]

Short Description

Updates our problematic command tests to use the tmp dir instead of a mocked filesystem(mock-fs)

Fixes #834

Implementation Details

1. Copies __modules__, __repo__ & __monorepo__ once because all test don't seem to write there. Just reads!
2. we have a function for updating the content of job.js during a test

QA Notes

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

• Code generation (copilot but not intellisense)
• Learning or fact checking
• Strategy / design
• Optimisation / refactoring
• Translation / spellchecking / doc gen
• Other
• I have not used AI

You can read more details in our Responsible AI Policy

[josephjclark]

Hey @doc-han , sorry I'm not sure I understand this one. Either I've missed or forgotten something or we've gotten a little lost in the woods.

The problem as I understand it is that in node 20, mock-fs still generally works, but it doesn't work for node import statements.

But since the test adaptors are all on disk anyway, surely we can just bypass the mock and load them "naturally"?

I don't really understand why we're copying files already on disk to a temporary directory, and I don't understand why we're writing job files to disk when the mock layer should still work.

So the only change should be that mock-fs doesn't mock out the repo or modules or any other dir that gets imported. So we mock inputs to the CLI but the module loader uses the real file system.

Is the issue more subtle than that? Like if we use mock-fs at all then import() totally dies?

I also think this branch should be targeted at your node 20 branch, because the node 20 behaviour is more important than the node 18 behaviour. Although I do appreciate that this proves the tests work after the refactor!

[doc-han]

So the only change should be that mock-fs doesn't mock out the repo or modules or any other dir that gets imported. So we mock inputs to the CLI but the module loader uses the real file system.

Is the issue more subtle than that? Like if we use mock-fs at all then import() totally dies?

Exactly. So mock-fs doesn't allow you have access to both the real and virtual(mocked) filesystem.
Hence, once you call mock-fs all your reads are to the virtual filesystem and you can't decide to pick other files from the real filesystem.

So for a given test, it's either you go fully real or virtual.

1. I'm doing the copying because I don't want to tamper with the stale(commited) data.
2. It happens only once, at start of all tests.

[josephjclark]

@doc-han isn't there a thing in mock-fs where you can ignore some paths and just let it access the filesystem? I'm sure we do that somewhere

[josephjclark]

Right: You can load real files and directories into the mock system using mock.load()

But I take it that still breaks import()?

[doc-han]

@doc-han isn't there a thing in mock-fs where you can ignore some paths and just let it access the filesystem? I'm sure we do that somewhere

It doesn't seem to work. mock-fs mocks out the filesystem.

[doc-han]

Right: You can load real files and directories into the mock system using mock.load()

But I take it that still breaks import()?

Yeah it breaks import().

[josephjclark]

Alright, thank you for the clarification.

Sorry to be a pain about this, and I totally understand the reasoning, but i just really don't like the solution 🤔 At least it's isolated to one test file.

What about we move the problematic tests into integration tests, which can use the filesystem in a more predictable way? They're all related to adaptor loading and deep runtime importing. They're actually hardly CLI tests at all - from a CLI point of view we just need to pass the right arguments into the runtime.

I don't think we'd even need to move every test - just look at the interesting ones. Some of them just test command line aliases (-a versus --adaptors), which really isn't very valuable at all

[doc-han]

Alright, thank you for the clarification.

Sorry to be a pain about this, and I totally understand the reasoning, but i just really don't like the solution 🤔 At least it's isolated to one test file.

What about we move the problematic tests into integration tests, which can use the filesystem in a more predictable way? They're all related to adaptor loading and deep runtime importing. They're actually hardly CLI tests at all - from a CLI point of view we just need to pass the right arguments into the runtime.

I don't think we'd even need to move every test - just look at the interesting ones. Some of them just test command line aliases (-a versus --adaptors), which really isn't very valuable at all

Yeah we can do that.

[josephjclark]

@doc-han awesome, thank you. Take a look - if you're not sure about what to move, a temporary solution is to .skip the offending tests and I'll move them next week.

[doc-han]

@doc-han awesome, thank you. Take a look - if you're not sure about what to move, a temporary solution is to .skip the offending tests and I'll move them next week.

I'll like to deal with them. How about moving just the offending test first (so that the node update branch can be progressed).

[josephjclark]

@doc-han sure - that sounds great. I'm all for fixing stuff nicely, just worried it's too boring for you!

Let's move the tests here. Anything ambiguous or difficult you can just flag to me and we'll work out what to do

[josephjclark]

Ok, this is getting too expensive. I'll do a final review ASAP and merge this as-is. Thanks!

Update: this is already in the base branch. Let's close this and keep it over there.