Repository: saltstack/salt
Pull request: fix npm installed 60339
### What does this PR do?

What it says on the tin, fixes #60339

### Previous Behavior

Previously the state only looked at the return type from the module to
determine if npm.install worked. 

### New Behavior

Since at some point npm changed their behavior, it makes more sense to just
rely on the retcode - 0 is success, anything else is failure.

### Merge requirements satisfied?
**[NOTICE] Bug fixes or features added to Salt require tests.**
<!-- Please review the [test documentation](https://docs.saltstack.com/en/master/topics/tutorials/writing_tests.html) for details on how to implement tests into Salt's test suite. -->
- [ ] Docs
- [x] Changelog - https://docs.saltstack.com/en/master/topics/development/changelog.html
- [x] Tests written/updated

### Commits signed with GPG?
Yes


Problem statement:
[BUG] npm.installed is broken on arch linux (possibly more)
**Description**
npm.installed reports a failure the first time it runs, even though it appears successful

**Setup**
```
npm:
  pkg.installed

bower:
  npm.installed
```
run this state on archlinux :point_up: 

**Steps to Reproduce the behavior**
sudo salt-call --local state.sls that-state-above

output is
```
local:
----------
          ID: npm
    Function: pkg.installed
      Result: True
     Comment: The following packages were installed/updated: npm
     Started: 22:24:26.528865
    Duration: 6219.277 ms
     Changes:   
              ----------
              c-ares:
                  ----------
                  new:
                      1.17.1-1
                  old:
              libuv:
                  ----------
                  new:
                      1.41.0-1
                  old:
              node-gyp:
                  ----------
                  new:
                      8.1.0-1
                  old:
              nodejs:
                  ----------
                  new:
                      16.3.0-1
                  old:
              npm:
                  ----------
                  new:
                      7.16.0-1
                  old:
              semver:
                  ----------
                  new:
                      7.3.5-2
                  old:
----------
          ID: bower
    Function: npm.installed
      Result: False
     Comment: Could not install package(s) 'bower'
     Started: 22:24:33.041832
    Duration: 3013.895 ms
     Changes:   

Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    1
------------

```

2nd time running it outputs:
```
local:
----------
          ID: npm
    Function: pkg.installed
      Result: True
     Comment: All specified packages are already installed
     Started: 22:26:04.890912
    Duration: 45.665 ms
     Changes:   
----------
          ID: bower
    Function: npm.installed
      Result: True
     Comment: Package(s) 'bower' satisfied by bower@1.8.12
     Started: 22:26:04.937660
    Duration: 682.688 ms
     Changes:   

Summary for local
------------
Succeeded: 2
Failed:    0
------------
Total states run:     2
Total run time: 728.353 ms
```

**Expected behavior**
It reports that it installed it successfully the first time

**Versions Report**
<details><summary>salt --versions-report</summary>

```
salt-call --versions
Salt Version:
          Salt: 3003
 
Dependency Versions:
          cffi: Not Installed
      cherrypy: Not Installed
      dateutil: Not Installed
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 3.0.1
       libgit2: Not Installed
      M2Crypto: 0.37.1
          Mako: Not Installed
       msgpack: 1.0.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: Not Installed
      pycrypto: Not Installed
  pycryptodome: 3.10.1
        pygit2: Not Installed
        Python: 3.9.5 (default, May 24 2021, 12:50:35)
  python-gnupg: Not Installed
        PyYAML: 5.4.1
         PyZMQ: 22.0.3
         smmap: Not Installed
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.4
 
System Versions:
          dist: arch  
        locale: utf-8
       machine: x86_64
       release: 5.11.0-16-generic
        system: Linux
       version: Arch Linux  
 
```
</details>

Hints from issue discussion:
Either this never worked or npm must have fixed a bug. https://github.com/saltstack/salt/pull/30657 added the ability to pass the silent flag - before that it was *always* used, which means it never would have produced the json output.

I don't understand that at all.

I can write a fix for this tomorrow.
Okay, looks like at some point in history, between 3.8.6 and 7, `npm --json --silent` went from producing json output to uh... not :facepalm:

Commit message: Skip install failure

Patch to review:
diff --git a/changelog/60339.fixed b/changelog/60339.fixed
new file mode 100644
index 000000000000..4d6a526d3d02
--- /dev/null
+++ b/changelog/60339.fixed
@@ -0,0 +1 @@
+Fixed faulty error message in npm.installed state.
diff --git a/salt/states/npm.py b/salt/states/npm.py
index 2afe84c0e44d..535a516a1d4e 100644
--- a/salt/states/npm.py
+++ b/salt/states/npm.py
@@ -210,16 +210,12 @@ def _pkg_is_installed(pkg, installed_pkgs):
         ret["result"] = False
         ret["comment"] = "Error installing '{}': {}".format(", ".join(pkg_list), err)
         return ret
-
-    if call and (isinstance(call, list) or isinstance(call, dict)):
+    else:
         ret["result"] = True
         ret["changes"] = {"old": [], "new": pkgs_to_install}
         ret["comment"] = "Package(s) '{}' successfully installed".format(
             ", ".join(pkgs_to_install)
         )
-    else:
-        ret["result"] = False
-        ret["comment"] = "Could not install package(s) '{}'".format(", ".join(pkg_list))
 
     return ret
 
diff --git a/tests/integration/states/test_npm.py b/tests/integration/states/test_npm.py
index 98331549f574..740630a0efe1 100644
--- a/tests/integration/states/test_npm.py
+++ b/tests/integration/states/test_npm.py
@@ -20,21 +20,7 @@
 
 @skipIf(salt.utils.path.which("npm") is None, "npm not installed")
 class NpmStateTest(ModuleCase, SaltReturnAssertsMixin):
-    @pytest.mark.requires_network
-    @pytest.mark.destructive_test
-    @pytest.mark.slow_test
-    def test_npm_installed_removed(self):
-        """
-        Basic test to determine if NPM module was successfully installed and
-        removed.
-        """
-        ret = self.run_state(
-            "npm.installed", name="pm2@2.10.4", registry="http://registry.npmjs.org/"
-        )
-        self.assertSaltTrueReturn(ret)
-        ret = self.run_state("npm.removed", name="pm2")
-        self.assertSaltTrueReturn(ret)
-
+    @skipIf(salt.utils.path.which("git") is None, "git is not installed")
     @skipIf(salt.utils.platform.is_darwin(), "TODO this test hangs on mac.")
     @pytest.mark.requires_network
     @pytest.mark.destructive_test
diff --git a/tests/pytests/functional/states/test_npm.py b/tests/pytests/functional/states/test_npm.py
new file mode 100644
index 000000000000..21a4e49ae393
--- /dev/null
+++ b/tests/pytests/functional/states/test_npm.py
@@ -0,0 +1,37 @@
+import pytest
+from salt.exceptions import CommandExecutionError
+
+
+@pytest.fixture(scope="module", autouse=True)
+def install_npm(sminion):
+    try:
+        sminion.functions.pkg.install("npm")
+    except CommandExecutionError:
+        pytest.skip("Unable to install npm")
+
+
+@pytest.mark.slow_test
+@pytest.mark.destructive_test
+@pytest.mark.requires_network
+def test_removed_installed_cycle(sminion):
+    project_version = "pm2@5.1.0"
+    success = sminion.functions.npm.uninstall("pm2")
+    assert success, "Unable to uninstall pm2 in prep for tests"
+
+    ret = next(
+        iter(
+            sminion.functions.state.single(
+                "npm.installed", name=project_version
+            ).values()
+        )
+    )
+    success = ret["result"]
+    assert success, "Failed to states.npm.installed " + project_version + ret["comment"]
+
+    ret = next(
+        iter(
+            sminion.functions.state.single("npm.removed", name=project_version).values()
+        )
+    )
+    success = ret["result"]
+    assert success, "Failed to states.npm.removed " + project_version
diff --git a/tests/pytests/unit/states/test_npm.py b/tests/pytests/unit/states/test_npm.py
index f4327a5b57d2..838fba19c28b 100644
--- a/tests/pytests/unit/states/test_npm.py
+++ b/tests/pytests/unit/states/test_npm.py
@@ -3,9 +3,10 @@
 """
 
 import pytest
+import salt.modules.npm as npmmod
 import salt.states.npm as npm
 from salt.exceptions import CommandExecutionError
-from tests.support.mock import MagicMock, patch
+from tests.support.mock import MagicMock, create_autospec, patch
 
 
 @pytest.fixture
@@ -13,6 +14,24 @@ def configure_loader_modules():
     return {npm: {"__opts__": {"test": False}}}
 
 
+@pytest.fixture(params=["", {}, []])
+def fake_install(request):
+    fake_install = create_autospec(npmmod.install, return_value=request.param)
+    with patch.dict(
+        npm.__salt__,
+        {
+            "npm.list": create_autospec(npmmod.list_, return_value={}),
+            "npm.install": fake_install,
+        },
+    ):
+        yield fake_install
+
+
+def test_when_install_does_not_error_installed_should_be_true(fake_install):
+    ret = npm.installed("fnord")
+    assert ret["result"] is True
+
+
 def test_installed():
     """
     Test to verify that the given package is installed


Code review comments:
diff_hunk: @@ -1,9 +1,13 @@
 import pytest
+from salt.exceptions import CommandExecutionError
 
 
 @pytest.fixture(scope="module", autouse=True)
 def install_npm(sminion):
-    sminion.functions.pkg.install("npm")
+    try:
+        sminion.functions.pkg.install("npm")
path: tests/pytests/functional/states/test_npm.py
text: @user1:
if we're going to install it, should we use pkg.installed so it won't upgrade if it's already installed?  I think that would greatly reduce the flakiness and still give us more of a chance that this doesn't accidentally get skipped.