Do not execute required mock

[royriojas]

Hi guys this is the improvement proposed as part of https://github.com/thlorenz/proxyquireify/issues/38

Couple of notes.

• the magic just wraps the call to require with an anonymous function. Since it is not executed it prevents double calls to require that were happening before.
• the tests were consistently failing, some sort of race condition maybe. I just solved it splitting the problematic test into its own module and launching it after the other tests. Would be really nice to figure it out what went wrong, but at least this helps pass the tests for now

[TheSavior]

I'd be surprised if this got merged. It clearly isn't the right solution to have to run it separately. I'm not familiar enough with the code either to know what is actually happening.

[royriojas]

The fix for the tests is not what I'm interested about, but for the one avoiding the require calls for mocks to be executed. I just wanted to be sure the code I introduced was not the guilty of the failures, as it clearly wasn't. The failure happens on browserify > 10.2.4. So it is probably something on their end and not in the tests

[royriojas]

I have found the way to fix the tests, it seems that there is some sort of race condition. The approach I have here is first compile the code, then run the tests, with that, it seems that the race condition went away and no need to have one of the tests to run alone

[thlorenz]

Please don't change any existing tests or how they are run. Otherwise we cannot be sure that no existing functionality is affected by your change. Thanks.

[royriojas]

@thlorenz this change required to change 1 of the tests because it no longer make sense. Since before you just put a require call the code of the module was executed immediately. this reported that the module was required 3 times, when in effect it was only required to be included twice.

The tests were broken (even without any changes) for browserify > 10.2.4

The change that I made was to make sure the tests were run sequentially without enter into race conditions which happened to fix the issues. I haven't change any other test, just make sure they were properly run in sequence

[bendrucker]

No need for promises to coordinate callback. run-parallel would do just fine on first glance.

[royriojas]

@sure, I'm not familiar with run-parallel if you point me to an example I can make the change if required.

But actually the problem is that tape need to run sequentially, not in parallel, to avoid the failure on browserify > 10.2.4

[bendrucker]

https://github.com/search?q=user%3Afeross+run-

[thlorenz]

Let's do this w/out adding any dependencies like this unless we have a good reason. A simple callback should work here.

Additionally don't change anything about the way the tests run please. I'm not sure why that would be required to add the feature, so you'd have to have a good reason and explain why you need this.

[thlorenz]

An example to wait for multiple parallel things to happen is here.

[royriojas]

@thlorenz sorry I updated the pull request before reading you don't want new dependencies. I used what @bendrucker suggested. run-parallel

So let's me explain why the test runner was modified.

1. If you pull your repo from master and run npm test you will see the tests are failing for browserify > 10.2.4
2. the issue seems to be that there is a race condition when entering the part that executes the code.
3. I realize that the first part of your code was actually creating the bundle, so in order to make things simpler, I moved the bundling part from the actual execution of the tests.

let me be clear that I haven't modified any tests, but the one that was incorrect: expecting the require to happen 3 times was not correct if the code only 2 calls to proxyquire were done. The extra one was for the magic require inserted.

Let me know if you want me to modify this to not use any dep, I will kindly ask you to consider run-parallel or even promises since they are dev dependencies not real dependencies.

Thanks for your time guys

[thlorenz]

Overall LGTM. Please fix the places I pointed out.

the issue seems to be that there is a race condition when entering the part that executes the code

We should check that that's not exposing a problem we need to fix in the code instead of just modifying the tests to cover it up.

WRT to run-parallel, although I'm convinced that this is totally overkill for what you're doing and it'd be a good exercise to write this with plain callbacks and task counting (unless it's obvious to you in which case it'll take a minute). However it'd fine either way and including it or not won't block this PR from my end.

[royriojas]

Hi @thlorenz,

I guess I have cover all of your observations. I do agree, this might be at least a minor or even major change, since it changes one of the unit tests.

I have removed run-parallel.

Thanks for your time

[thlorenz]

LGTM once the last few nits are fixed. Gonna wait for @bendrucker to review before merging.

[bendrucker]

Made some small stylistic comments, LGTM once they're resolved

[royriojas]

All but the comment for other coverage tools are resolved. Each coverage tool has different ways to ignore block of codes.

I imagine that maybe a solution would be to actually register a module using b.require(module), but them we will have to find a way to register all the possible aliases during bundling.

That would require going back to the drawing board, so I guess is out of the scope of this pull request. I guess for now this is good enough

[bendrucker]

I don't really use coverage tools these days. Can you give some examples?

[royriojas]

Hi @bendrucker,

the comment instruction I put there was for babel-istanbul, I used with browserify-babel-istanbul they use /* istanbul ignore next */

I couldn't find info about any other coverage tool, blanketjs is the other one I know, but that doesn't have a way to ignore the generated code.

[bendrucker]

Ok, I think it's fine to leave it for now

[thlorenz]

Noted some more minor nits in last last commit, please fix and we can merge if @bendrucker is ok with everything.

BTW thanks for your patience with nit fixing, but we need to uphold current style in module.

[royriojas]

@thlorenz I made the requested changes. BTW Thanks for writing the module in the first place.

[bendrucker]

Looks okay to me, pending a squash

[royriojas]

done!

[thlorenz]

So is this good to merge @bendrucker? If yes I'll try to get to it tomorrow.

[bendrucker]

Yes!

[thlorenz]

Ok, merged 0d5fe82f00b2a9e12c138f59544569cae1fb220d and published as v3. This is a major upgrade as discussed due to the breaking change in behavior.

[bendrucker]

@royriojas Can you do me a favor and send a PR to proxyquire-universal updating the peerDep?

[royriojas]

sure @bendrucker, will do.

[royriojas]

@bendrucker It seems that proxyquire-universal was pin to use 1.x.x versions of proxyquireify. Tests fails if I even try to use version 2.0.0 of proxyquireify. That will require change on the tests on proxyquire-universal. Are you open to do that? Wouldn't it be better to create a new issue for the update?

[bendrucker]

Yeah sure, really appreciate it

[thlorenz]

@royriojas I'd just create a PR over there with what you have so far (think of a PR as an issue with code). Then you guys can figure out how to solve the problem.

[royriojas]

@thlorenz ok will open then the PR. Test won't pass though