fix testonly with js_binary so it's not a test

[Aghassi]

the current logic is wrong because it tries to test js_binary when testonly is passed. We actually want the inverse which is we don't want to run js_binary as a test if it is marked as test_only.

Bug discovered by myself and @joeljeske

---

Type of change

• Bug fix (change which fixes an issue)

For changes visible to end-users

• Breaking change (this change will force users to change their own code or config)
• Relevant documentation has been updated
• Suggested release notes are provided below:

Test plan

• Covered by existing test cases
• New test cases added
• Manual testing; please provide instructions so we can reproduce:

[gregmagolan]

Interesting one. Suppose there is no value of tagging a js_binary as testonly. I'm still confused how you get this to fail since neither bazel test or bazel coverage should run a js_binary target as test right?

[jbedard]

Can you show an example that fails without the change?

[joeljeske]

We have a js_binary target that is not a test itself, but a dependency of a test. The testonly here is a guard against accidental usage outside of a test dag.

Presumably you could fix this more properly by having js_test also have a _is_test attr that is set to true. This would allow you to ensure all js_tests have a lcov_merger but allow a js_binary to be used with testonly = True.

[jbedard]

Can that be shown with a test that fails without this change though?

[Aghassi]

@jbedard here is a screenshot from me reproducing the failure with bazel coverage using vscode codespaces off of main

Here is the code

# A simple program that runs the Acorn JS parser to produce an AST
js_binary(
    name = "my_bin",
    # Reference the location where the acorn npm module was linked in the root Bazel package
    data = ["//:node_modules/acorn"],
    entry_point = "require_acorn.js",
    testonly = True
)

js_test(name = "test_testonly_bin", data = ["my_bin"], entry_point = "npm_version_test.js")

[Aghassi]

I can confirm also that applying this fix with that code fixes the bug

[thesayyn]

i believe this is still wrong as _lcov_merger should be present if the coverage is enabled but since we are sharing implementation between js_test and js_binary we need a sentinel attribute for determining where the impl is being invoked so we can still fail if it's js_test.

[Aghassi]

@thesayyn so are you suggesting that @joeljeske's fix would be preferred?

Presumably you could fix this more properly by having js_test also have a _is_test attr that is set to true. This would allow you to ensure all js_tests have a lcov_merger but allow a js_binary to be used with testonly = True.

#1486 (comment)

[thesayyn]

Exactly.

[Aghassi]

@thesayyn Ok I believe I have added the fix as interpreted from @joeljeske's comment. I confirmed with the same test as before that this fix works too

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Aghassi]

Looks like this breaks some CI stuff. I'll have to spin up an ubuntu container and debug why bazel coverage //... is failing