Repository: saltstack/salt
Pull request: port batch tests and fix returncode logic
### What does this PR do?
Fixes retcode logic with batch execution and ports relevant tests to pytest

### What issues does this PR fix or reference?
Fixes: https://github.com/saltstack/salt/issues/60361

### Previous Behavior
Wrong retcode (or error)

### New Behavior
Correct retcode and no error

### Merge requirements satisfied?
**[NOTICE] Bug fixes or features added to Salt require tests.**
<!-- Please review the [test documentation](https://docs.saltproject.io/en/master/topics/tutorials/writing_tests.html) for details on how to implement tests into Salt's test suite. -->
- [ ] Docs
- [x] Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
- [x] Tests written/updated

### Commits signed with GPG?
Yes


Problem statement:
[BUG] Batch execution of multiple modules fails with salt global exception
**Description**
Batched running of multiple modules fails due to TypeError exception.  Both batch.py and salt.py assume a single return code for batched execution runs, but running multiple modules results in a dictionary of return codes.

Applied the following patches to job.py and salt.py and they appear to fix the issue
```
root@use1-salt-1:/usr/lib/python3/dist-packages/salt/cli# diff -u `pwd`/salt.py  /root/salt.py_patched
diff --git a/cli/salt.py b/cli/salt.py
index c672130..f3c8789 100644
--- a/cli/salt.py
+++ b/cli/salt.py
@@ -299,6 +299,13 @@ class SaltCMD(salt.utils.parsers.SaltCMDOptionParser):
             for res in batch.run():
                 for ret in res.values():
                     job_retcode = salt.utils.job.get_retcode(ret)
+                    # handle batched running of multiple modules
+                    if isinstance(job_retcode, dict):
+                        try:
+                            job_retcode = max(job_retcode.values())
+                        # handle no retcode values in dict
+                        except ValueError:
+                            job_retcode = 0
                     if job_retcode > retcode:
                         # Exit with the highest retcode we find
                         retcode = job_retcode


--- /root/job.py        2021-01-14 09:16:03.332204018 -0500
+++ /usr/lib/python3/dist-packages/salt/utils/job.py    2021-01-14 09:10:58.415043396 -0500
@@ -174,8 +174,14 @@
     """
     retcode = 0
     # if there is a dict with retcode, use that
-    if isinstance(ret, dict) and ret.get("retcode", 0) != 0:
-        return ret["retcode"]
+    if isinstance(ret, dict):
+        retcode = ret.get("retcode", 0)
+        if isinstance(retcode, dict):
+            for retcode in retcode.values():
+                if retcode != 0:
+                    return retcode
+        elif retcode != 0:
+            return retcode
     # if its a boolean, False means 1
     elif isinstance(ret, bool) and not ret:
         return 1

```

**Setup**
Salt master and multiple syndics all running 3003.
Minions running 3003

**Steps to Reproduce the behavior**
Run a batch command with multiple modules:
salt --no-color --hide-timeout --timeout 30 --out json --out-file /tmp/cldeng-20519.txt  '*'  -b 50 virt.full_info,cmd.run ,'/bin/true'

Get the following stack trace:
```
[ERROR   ] An un-handled exception was caught by salt's global exception handler:
TypeError: '>' not supported between instances of 'dict' and 'int'
Traceback (most recent call last):
  File "/usr/bin/salt", line 11, in <module>
    load_entry_point('salt==3003', 'console_scripts', 'salt')()
  File "/usr/lib/python3/dist-packages/salt/scripts.py", line 539, in salt_main
    client.run()
  File "/usr/lib/python3/dist-packages/salt/cli/salt.py", line 61, in run
    self._run_batch()
  File "/usr/lib/python3/dist-packages/salt/cli/salt.py", line 302, in _run_batch
    if job_retcode > retcode:
TypeError: '>' not supported between instances of 'dict' and 'int'
Traceback (most recent call last):
  File "/usr/bin/salt", line 11, in <module>
    load_entry_point('salt==3003', 'console_scripts', 'salt')()
  File "/usr/lib/python3/dist-packages/salt/scripts.py", line 539, in salt_main
    client.run()
  File "/usr/lib/python3/dist-packages/salt/cli/salt.py", line 61, in run
    self._run_batch()
  File "/usr/lib/python3/dist-packages/salt/cli/salt.py", line 302, in _run_batch
    if job_retcode > retcode:
TypeError: '>' not supported between instances of 'dict' and 'int'
```
(Include debug logs if possible and relevant)

**Expected behavior**
Commands return successfully and report the correct return code (highest value returned)

**Screenshots**
If applicable, add screenshots to help explain your problem.

**Versions Report**
<details><summary>salt --versions-report</summary>
(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.) 

```
# salt --versions-report
Salt Version:
          Salt: 3003

Dependency Versions:
          cffi: Not Installed
      cherrypy: unknown
      dateutil: 2.6.1
     docker-py: Not Installed
         gitdb: 2.0.3
     gitpython: 2.1.8
        Jinja2: 2.10
       libgit2: Not Installed
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 0.5.6
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: Not Installed
      pycrypto: Not Installed
  pycryptodome: 3.4.7
        pygit2: Not Installed
        Python: 3.6.9 (default, Apr 18 2020, 01:56:04)
  python-gnupg: 0.4.1
        PyYAML: 3.12
         PyZMQ: 17.1.2
         smmap: 2.0.3
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.2.5

System Versions:
          dist: ubuntu 18.04 Bionic Beaver
        locale: UTF-8
       machine: x86_64
       release: 4.15.0-99-generic
        system: Linux
       version: Ubuntu 18.04 Bionic Beaver

```
</details>

**Additional context**
Add any other context about the problem here.

Hints from issue discussion:
Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our [Code of Conduct](https://github.com/saltstack/salt/blob/master/CODE_OF_CONDUCT.md). Also, check out some of our community resources including:

  - [Community Wiki](https://github.com/saltstack/community/wiki)
  - [Salt’s Contributor Guide](https://docs.saltproject.io/en/master/topics/development/contributing.html)
  - [Join our Community Slack](https://join.slack.com/t/saltstackcommunity/shared_invite/zt-3av8jjyf-oBQ2M0vhXOhJpNpRkPWBvg)
  - [IRC on LiberaChat](https://web.libera.chat/#salt)
  - [Salt Project YouTube channel](https://www.youtube.com/channel/UCpveTIucFx9ljGelW63-BWg)
  - [Salt Project Twitch channel](https://www.twitch.tv/saltprojectoss)

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

Verified this using this command

```
salt \* -b 1 test.ping,test.version ,'/bin/true'
```

I get the same result.

Looking at the code, `job.py` has basically been unchanged, however there have been a few changes to `batch.py`. I'm not positive if that's where things went a bit sideways, but it looks like the return isn't returning the expected sort of value :thinking: 

This fix *would* stop the exception, but AFAICT this *should* be working (though, I couldn't get back to a point where it was working, so it's possible that it never did? :shrug: )
Digging into this a bit more, it's definitely due to `batch.py` getting horked up. In fact, replacing parts of the existing `batch.py` -- specifically if you put this block in:

https://github.com/saltstack/salt/blob/ead47e4bba6d8addd0717b28b81251e3a4a22647/salt/cli/batch.py#L222-L247

No other changes are necessary - that will make this existing code work. Granted, I don't think that's a comprehensive fix, but it *does* solve this particular problem.

Some time between then and now, that block got changed in an unexpected way, causing the failure that we're seeing. From what I can see, the correct fix would be to add some tests to verify that run is, in fact, returning expected data not just for the cases where there's a single return, but for multiple returns and every case, and ensure that all of the expected data is being returned. Naturally there should be some failures based on the current state, but... that's where this fix should happen :+1: 
I was going to test the above batch.py to see if it would solve this bug, but that commit appears to be very old (2016ish) -- the batch.py file supplied with Salt 3003 is very different (https://github.com/saltstack/salt/blob/9242a14ebc204044cc585f337591cf85d997501e/salt/cli/batch.py)
Is there any more information I can provide here?  Not clear on next steps.
@bzukdatto I have verified this is still an issue with

`salt * -b 50 test.ping,test.echo ,nacl`

and will start working on a fix, using what you and @waynew have already said as a launching pad.
Actually, I've confirmed that normal retcodes are also returning incorrectly in normal scenarios.  It seems, as @waynew stated, batch.py got messed up somewhere in the past, and I'd like to keep the fix local to that file if able since it really is a problem with how it operates.

Commit message: Merge branch 'master' into batch-failure

Patch to review:
diff --git a/changelog/60361.fixed b/changelog/60361.fixed
new file mode 100644
index 000000000000..fad476428a77
--- /dev/null
+++ b/changelog/60361.fixed
@@ -0,0 +1 @@
+Batch runs now return proper retcodes in a tuple of the form (result, retcode)
diff --git a/salt/cli/batch.py b/salt/cli/batch.py
index cc934e0dc2a5..755dbd13d423 100644
--- a/salt/cli/batch.py
+++ b/salt/cli/batch.py
@@ -275,26 +275,26 @@ def run(self):
                     active.remove(minion)
                     if bwait:
                         wait.append(datetime.now() + timedelta(seconds=bwait))
-                # Munge retcode into return data
                 failhard = False
-                if (
-                    "retcode" in data
-                    and isinstance(data["ret"], dict)
-                    and "retcode" not in data["ret"]
-                ):
-                    data["ret"]["retcode"] = data["retcode"]
-                    if self.opts.get("failhard") and data["ret"]["retcode"] > 0:
-                        failhard = True
-                else:
+                # If we are executing multiple modules with the same cmd,
+                # We use the highest retcode.
+                retcode = 0
+                if "retcode" in data:
+                    if isinstance(data["retcode"], dict):
+                        try:
+                            data["retcode"] = max(data["retcode"].values())
+                        except ValueError:
+                            data["retcode"] = 0
                     if self.opts.get("failhard") and data["retcode"] > 0:
                         failhard = True
+                    retcode = data["retcode"]
 
                 if self.opts.get("raw"):
                     ret[minion] = data
-                    yield data
+                    yield data, retcode
                 else:
                     ret[minion] = data["ret"]
-                    yield {minion: data["ret"]}
+                    yield {minion: data["ret"]}, retcode
                 if not self.quiet:
                     ret[minion] = data["ret"]
                     data[minion] = data.pop("ret")
diff --git a/salt/cli/salt.py b/salt/cli/salt.py
index 6566a3070b3b..0c0a25bb9802 100644
--- a/salt/cli/salt.py
+++ b/salt/cli/salt.py
@@ -3,7 +3,6 @@
 
 import salt.defaults.exitcodes
 import salt.log
-import salt.utils.job
 import salt.utils.parsers
 import salt.utils.stringutils
 from salt.exceptions import (
@@ -280,7 +279,7 @@ def _run_batch(self):
 
             ret = {}
 
-            for res in batch.run():
+            for res, _ in batch.run():
                 ret.update(res)
 
             self._output_ret(ret, "")
@@ -296,12 +295,10 @@ def _run_batch(self):
                 sys.exit(1)
             # Printing the output is already taken care of in run() itself
             retcode = 0
-            for res in batch.run():
-                for ret in res.values():
-                    job_retcode = salt.utils.job.get_retcode(ret)
-                    if job_retcode > retcode:
-                        # Exit with the highest retcode we find
-                        retcode = job_retcode
+            for res, job_retcode in batch.run():
+                if job_retcode > retcode:
+                    # Exit with the highest retcode we find
+                    retcode = job_retcode
             sys.exit(retcode)
 
     def _print_errors_summary(self, errors):
diff --git a/salt/client/__init__.py b/salt/client/__init__.py
index b50dd9da44ed..605b956ce0d7 100644
--- a/salt/client/__init__.py
+++ b/salt/client/__init__.py
@@ -629,7 +629,7 @@ def cmd_batch(
             if key not in opts:
                 opts[key] = val
         batch = salt.cli.batch.Batch(opts, eauth=eauth, quiet=True)
-        for ret in batch.run():
+        for ret, _ in batch.run():
             yield ret
 
     def cmd(
diff --git a/tests/integration/cli/test_batch.py b/tests/integration/cli/test_batch.py
deleted file mode 100644
index e3d645f13189..000000000000
--- a/tests/integration/cli/test_batch.py
+++ /dev/null
@@ -1,144 +0,0 @@
-"""
-    :codeauthor: Nicole Thomas <nicole@saltstack.com>
-"""
-
-import pytest
-import salt.utils.platform
-from tests.support.case import ShellCase
-
-
-@pytest.mark.windows_whitelisted
-@pytest.mark.usefixtures("salt_sub_minion")
-class BatchTest(ShellCase):
-    """
-    Integration tests for the salt.cli.batch module
-    """
-
-    if salt.utils.platform.is_windows():
-        run_timeout = 180
-    else:
-        run_timeout = 30
-
-    @pytest.mark.slow_test
-    def test_batch_run(self):
-        """
-        Tests executing a simple batch command to help catch regressions
-        """
-        ret = "Executing run on [{}]".format(repr("sub_minion"))
-        cmd = self.run_salt(
-            '"*minion" test.echo "batch testing" -b 50%',
-            timeout=self.run_timeout,
-        )
-        self.assertIn(ret, cmd)
-
-    @pytest.mark.slow_test
-    def test_batch_run_number(self):
-        """
-        Tests executing a simple batch command using a number division instead of
-        a percentage with full batch CLI call.
-        """
-        ret = "Executing run on [{}, {}]".format(repr("minion"), repr("sub_minion"))
-        cmd = self.run_salt(
-            '"*minion" test.ping --batch-size 2',
-            timeout=self.run_timeout,
-        )
-        self.assertIn(ret, cmd)
-
-    @pytest.mark.slow_test
-    def test_batch_run_grains_targeting(self):
-        """
-        Tests executing a batch command using a percentage divisor as well as grains
-        targeting.
-        """
-        os_grain = ""
-        sub_min_ret = "Executing run on [{}]".format(repr("sub_minion"))
-        min_ret = "Executing run on [{}]".format(repr("minion"))
-
-        for item in self.run_salt("minion grains.get os"):
-            if item != "minion:":
-                os_grain = item
-
-        os_grain = os_grain.strip()
-        cmd = self.run_salt(
-            '-C "G@os:{} and not localhost" -b 25% test.ping'.format(
-                os_grain.replace(" ", "?")
-            ),
-            timeout=self.run_timeout,
-        )
-        self.assertIn(sub_min_ret, cmd)
-        self.assertIn(min_ret, cmd)
-
-    @pytest.mark.slow_test
-    def test_batch_exit_code(self):
-        """
-        Test that a failed state returns a non-zero exit code in batch mode
-        """
-        cmd = self.run_salt(
-            ' "*" state.single test.fail_without_changes name=test_me -b 25%',
-            with_retcode=True,
-            timeout=self.run_timeout,
-        )
-        self.assertEqual(cmd[-1], 2)
-
-    # Test for failhard + batch. The best possible solution here was to do something like that:
-    # assertRaises(StopIteration)
-    # But it's impossible due to nature of the tests execution via fork()
-
-    @pytest.mark.slow_test
-    def test_batch_module_stopping_after_error(self):
-        """
-        Test that a failed command stops the batch run
-        """
-
-        minions_list = []
-        retcode = None
-
-        # Executing salt with batch: 1 and with failhard. It should stop after the first error.
-        cmd = self.run_salt(
-            '"*minion" test.retcode 42 -b 1 --out=yaml --failhard',
-            timeout=self.run_timeout,
-        )
-
-        # Parsing the output. Idea is to fetch number on minions and retcode of the execution.
-        # retcode var could be overwritten in case of broken failhard but number of minions check should still fail.
-        for line in cmd:
-            if line.startswith("Executing run on"):
-                minions_list.append(line)
-            if line.startswith("retcode"):
-                retcode = line[-1]
-        # We expect to have only one minion to be run
-        self.assertEqual(1, len(minions_list))
-        # We expect to find a retcode in the output
-        self.assertIsNot(None, retcode)
-        # We expect retcode to be non-zero
-        self.assertNotEqual(0, retcode)
-
-    @pytest.mark.slow_test
-    def test_batch_state_stopping_after_error(self):
-        """
-        Test that a failed state stops the batch run
-        """
-
-        minions_list = []
-        retcode = None
-
-        # Executing salt with batch: 1 and with failhard. It should stop after the first error.
-        cmd = self.run_salt(
-            '"*minion" state.single test.fail_without_changes name=test_me -b 1'
-            " --out=yaml --failhard",
-            timeout=self.run_timeout,
-        )
-
-        # Parsing the output. Idea is to fetch number on minions and retcode of the execution.
-        # retcode var could be overwritten in case of broken failhard but number of minions check should still fail.
-        for line in cmd:
-            if line.startswith("Executing run on"):
-                minions_list.append(line)
-            if line.startswith("retcode"):
-                retcode = line[-1]
-        # We expect to have only one minion to be run
-        self.assertEqual(1, len(minions_list))
-        # We expect to find a retcode in the output
-        self.assertIsNot(None, retcode)
-        # We expect retcode to be non-zero
-        self.assertNotEqual(0, retcode)
diff --git a/tests/pytests/functional/cli/test_batch.py b/tests/pytests/functional/cli/test_batch.py
index 1a19bdf7d732..e721b729cfe4 100644
--- a/tests/pytests/functional/cli/test_batch.py
+++ b/tests/pytests/functional/cli/test_batch.py
@@ -178,7 +178,7 @@ def returns_for_job(jid):
             ):
                 ret = list(batch.run())
     assert len(ret) == 4
-    for val in ret:
+    for val, _ in ret:
         values = list(val.values())
         assert len(values) == 1
         assert values[0] is True
diff --git a/tests/pytests/integration/cli/test_batch.py b/tests/pytests/integration/cli/test_batch.py
new file mode 100644
index 000000000000..ddffe554780e
--- /dev/null
+++ b/tests/pytests/integration/cli/test_batch.py
@@ -0,0 +1,203 @@
+"""
+    :codeauthor: Nicole Thomas <nicole@saltstack.com>
+"""
+
+import pytest
+import salt.utils.platform
+
+pytestmark = [
+    pytest.mark.windows_whitelisted,
+    pytest.mark.slow_test,
+]
+
+
+@pytest.fixture(scope="module")
+def run_timeout():
+    if salt.utils.platform.is_windows():
+        return 180
+    else:
+        return 30
+
+
+def test_batch_run(salt_cli, run_timeout, salt_sub_minion):
+    """
+    Tests executing a simple batch command to help catch regressions
+    """
+    ret = "Executing run on [{}]".format(repr(salt_sub_minion.id))
+    cmd = salt_cli.run(
+        "test.echo",
+        "batch testing",
+        "-b 50%",
+        minion_tgt="*minion*",
+        _timeout=run_timeout,
+    )
+    assert ret in cmd.stdout
+
+
+def test_batch_run_number(salt_cli, salt_minion, salt_sub_minion, run_timeout):
+    """
+    Tests executing a simple batch command using a number division instead of
+    a percentage with full batch CLI call.
+    """
+    ret = "Executing run on [{}, {}]".format(
+        repr(salt_minion.id), repr(salt_sub_minion.id)
+    )
+    cmd = salt_cli.run(
+        "--batch-size=2",
+        "test.ping",
+        minion_tgt="*minion*",
+        _timeout=run_timeout,
+    )
+    assert ret in cmd.stdout
+
+
+def test_batch_run_grains_targeting(
+    salt_cli, salt_minion, salt_sub_minion, run_timeout
+):
+    """
+    Tests executing a batch command using a percentage divisor as well as grains
+    targeting.
+    """
+    sub_min_ret = "Executing run on [{}]".format(repr(salt_sub_minion.id))
+    min_ret = "Executing run on [{}]".format(repr(salt_minion.id))
+    os_grain = salt_cli.run("grains.get", "os", minion_tgt=salt_minion.id).json
+
+    os_grain = os_grain.strip()
+    cmd = salt_cli.run(
+        "-C",
+        "-b 25%",
+        "test.ping",
+        minion_tgt="G@os:{} and not localhost".format(os_grain.replace(" ", "?")),
+        _timeout=run_timeout,
+    )
+    assert sub_min_ret in cmd.stdout
+    assert min_ret in cmd.stdout
+
+
+def test_batch_exit_code(salt_cli, salt_minion, salt_sub_minion, run_timeout):
+    """
+    Test that a failed state returns a non-zero exit code in batch mode
+    """
+    cmd = salt_cli.run(
+        "state.single",
+        "test.fail_without_changes",
+        "name=test_me",
+        "-b 25%",
+        minion_tgt="*minion*",
+        _timeout=run_timeout,
+    )
+    assert cmd.exitcode == 2
+
+
+# Test for failhard + batch. The best possible solution here was to do something like that:
+# assertRaises(StopIteration)
+# But it's impossible due to nature of the tests execution via fork()
+
+
+def test_batch_module_stopping_after_error(
+    salt_cli, salt_minion, salt_sub_minion, run_timeout
+):
+    """
+    Test that a failed command stops the batch run
+    """
+
+    minions_list = []
+    retcode = None
+
+    # Executing salt with batch: 1 and with failhard. It should stop after the first error.
+    cmd = salt_cli.run(
+        "test.retcode",
+        42,
+        "-b 1",
+        "--out=yaml",
+        "--failhard",
+        minion_tgt="*minion*",
+        _timeout=run_timeout,
+    )
+
+    # Parsing the output. Idea is to fetch number on minions and retcode of the execution.
+    # retcode var could be overwritten in case of broken failhard but number of minions check should still fail.
+    for line in cmd.stdout.splitlines():
+        line = line.strip()
+        if line.startswith("Executing run on"):
+            minions_list.append(line)
+        if line.startswith("retcode"):
+            retcode = int(line.split(" ")[-1])
+    # We expect to have only one minion to be run
+    assert 1 == len(minions_list)
+    # We expect to find a retcode in the output
+    assert None is not retcode
+    # We expect retcode to be non-zero
+    assert 0 != retcode
+
+
+def test_batch_state_stopping_after_error(
+    salt_cli, salt_minion, salt_sub_minion, run_timeout
+):
+    """
+    Test that a failed state stops the batch run
+    """
+
+    minions_list = []
+    retcode = None
+
+    # Executing salt with batch: 1 and with failhard. It should stop after the first error.
+    cmd = salt_cli.run(
+        "state.single" "test.fail_without_changes" "name=test_me",
+        "-b 1",
+        "--out=yaml",
+        "--failhard",
+        minion_tgt="*minion*",
+        _timeout=run_timeout,
+    )
+
+    # Parsing the output. Idea is to fetch number on minions and retcode of the execution.
+    # retcode var could be overwritten in case of broken failhard but number of minions check should still fail.
+    for line in cmd.stdout.splitlines():
+        if line.startswith("Executing run on"):
+            minions_list.append(line)
+        if line.startswith("retcode"):
+            retcode = int(line.split(" ")[-1])
+    # We expect to have only one minion to be run
+    assert 1 == len(minions_list)
+    # We expect to find a retcode in the output
+    assert None is not retcode
+    # We expect retcode to be non-zero
+    assert 0 != retcode
+
+
+def test_batch_retcode(salt_cli, salt_minion, salt_sub_minion, run_timeout):
+    """
+    Ensure that running multiple modules at the same time works in batch.
+
+    See issue #60361
+    """
+    cmd = salt_cli.run(
+        "test.retcode",
+        "23",
+        "-b 1",
+        minion_tgt="*minion*",
+        _timeout=run_timeout,
+    )
+
+    assert cmd.exitcode == 23
+    assert not cmd.stderr
+    assert "true" in cmd.stdout
+
+
+def test_multiple_modules_in_batch(salt_cli, salt_minion, salt_sub_minion, run_timeout):
+    """
+    Ensure that running multiple modules at the same time works in batch.
+
+    See issue #60361
+    """
+    cmd = salt_cli.run(
+        "test.ping,test.retcode",
+        ",23",
+        "-b 1",
+        minion_tgt="*minion*",
+        _timeout=run_timeout,
+    )
+
+    assert cmd.exitcode == 23
+    assert not cmd.stderr


Code review comments:
diff_hunk: @@ -0,0 +1,203 @@
+"""
+    :codeauthor: Nicole Thomas <nicole@saltstack.com>
+"""
+
+import pytest
+import salt.utils.platform
+
+pytestmark = [
+    pytest.mark.windows_whitelisted,
+    pytest.mark.slow_test,
+]
+
+
+@pytest.fixture(scope="module")
+def run_timeout():
+    if salt.utils.platform.is_windows():
+        return 180
+    else:
+        return 30
+
+
+def test_batch_run(salt_cli, run_timeout, salt_sub_minion):
+    """
+    Tests executing a simple batch command to help catch regressions
+    """
+    ret = "Executing run on [{}]".format(repr(salt_sub_minion.id))
+    cmd = salt_cli.run(
+        "test.echo",
+        "batch testing",
+        "-b 50%",
+        minion_tgt="*minion*",
+        _timeout=run_timeout,
+    )
+    assert ret in cmd.stdout
+
+
+def test_batch_run_number(salt_cli, salt_minion, salt_sub_minion, run_timeout):
+    """
+    Tests executing a simple batch command using a number division instead of
+    a percentage with full batch CLI call.
+    """
+    ret = "Executing run on [{}, {}]".format(
+        repr(salt_minion.id), repr(salt_sub_minion.id)
+    )
+    cmd = salt_cli.run(
+        "--batch-size=2",
+        "test.ping",
+        minion_tgt="*minion*",
+        _timeout=run_timeout,
+    )
+    assert ret in cmd.stdout
+
+
+def test_batch_run_grains_targeting(
+    salt_cli, salt_minion, salt_sub_minion, run_timeout
+):
+    """
+    Tests executing a batch command using a percentage divisor as well as grains
+    targeting.
+    """
+    sub_min_ret = "Executing run on [{}]".format(repr(salt_sub_minion.id))
+    min_ret = "Executing run on [{}]".format(repr(salt_minion.id))
+    os_grain = salt_cli.run("grains.get", "os", minion_tgt=salt_minion.id).json
+
+    os_grain = os_grain.strip()
+    cmd = salt_cli.run(
+        "-C",
+        "-b 25%",
+        "test.ping",
+        minion_tgt="G@os:{} and not localhost".format(os_grain.replace(" ", "?")),
+        _timeout=run_timeout,
+    )
+    assert sub_min_ret in cmd.stdout
+    assert min_ret in cmd.stdout
+
+
+def test_batch_exit_code(salt_cli, salt_minion, salt_sub_minion, run_timeout):
+    """
+    Test that a failed state returns a non-zero exit code in batch mode
+    """
+    cmd = salt_cli.run(
+        "state.single",
+        "test.fail_without_changes",
+        "name=test_me",
+        "-b 25%",
+        minion_tgt="*minion*",
+        _timeout=run_timeout,
+    )
+    assert cmd.exitcode == 2
+
+
+# Test for failhard + batch. The best possible solution here was to do something like that:
+# assertRaises(StopIteration)
+# But it's impossible due to nature of the tests execution via fork()
+
+
+def test_batch_module_stopping_after_error(
+    salt_cli, salt_minion, salt_sub_minion, run_timeout
+):
+    """
+    Test that a failed command stops the batch run
+    """
+
+    minions_list = []
+    retcode = None
+
+    # Executing salt with batch: 1 and with failhard. It should stop after the first error.
+    cmd = salt_cli.run(
+        "test.retcode",
+        42,
+        "-b 1",
+        "--out=yaml",
+        "--failhard",
+        minion_tgt="*minion*",
+        _timeout=run_timeout,
+    )
+
+    # Parsing the output. Idea is to fetch number on minions and retcode of the execution.
+    # retcode var could be overwritten in case of broken failhard but number of minions check should still fail.
+    for line in cmd.stdout.splitlines():
+        line = line.strip()
+        if line.startswith("Executing run on"):
+            minions_list.append(line)
+        if line.startswith("retcode"):
+            retcode = int(line.split(" ")[-1])
+    # We expect to have only one minion to be run
+    assert 1 == len(minions_list)
+    # We expect to find a retcode in the output
+    assert None is not retcode
+    # We expect retcode to be non-zero
+    assert 0 != retcode
+
+
+def test_batch_state_stopping_after_error(
+    salt_cli, salt_minion, salt_sub_minion, run_timeout
+):
+    """
+    Test that a failed state stops the batch run
+    """
+
+    minions_list = []
+    retcode = None
+
+    # Executing salt with batch: 1 and with failhard. It should stop after the first error.
+    cmd = salt_cli.run(
+        "state.single" "test.fail_without_changes" "name=test_me",
+        "-b 1",
+        "--out=yaml",
+        "--failhard",
+        minion_tgt="*minion*",
+        _timeout=run_timeout,
+    )
+
+    # Parsing the output. Idea is to fetch number on minions and retcode of the execution.
+    # retcode var could be overwritten in case of broken failhard but number of minions check should still fail.
+    for line in cmd.stdout.splitlines():
+        if line.startswith("Executing run on"):
+            minions_list.append(line)
+        if line.startswith("retcode"):
+            retcode = int(line.split(" ")[-1])
+    # We expect to have only one minion to be run
+    assert 1 == len(minions_list)
+    # We expect to find a retcode in the output
+    assert None is not retcode
+    # We expect retcode to be non-zero
+    assert 0 != retcode
+
+
+def test_batch_retcode(salt_cli, salt_minion, salt_sub_minion, run_timeout):
+    """
+    Ensure that running multiple modules at the same time works in batch.
path: tests/pytests/integration/cli/test_batch.py
text: @user1:
We're not running multiple modules in this test