Improve our CI tests (qmk#11476)

* add a test and dry-run to qmk generate-api

* add a dry-run to qmk pyformat

* Add a --dry-run to qmk cformat

* reverse the order of nose2 and flake8 tests

* run CI test against cformat and pyformat

* fix programming errors

* tweak job name

* fix argument

* refine the files we select

* fix stack trace in --ci

* make cformat exit clean

* fix c file extensions

* decouple CI from pyformat

* remove --ci arg

* make ci happy

* use the environment var instead

* change output to text

* fix log message

* replace tabs

.github/workflows/format.yaml

@@ -1,47 +1,42 @@
-name: Format Codebase
+name: PR Lint Format
 
 on:
-  push:
-    branches:
-    - master
-    - develop
+  pull_request:
+    paths:
+    - 'drivers/**'
+    - 'lib/arm_atsam/**'
+    - 'lib/lib8tion/**'
+    - 'lib/python/**'
+    - 'platforms/**'
+    - 'quantum/**'
+    - 'tests/**'
+    - 'tmk_core/**'
 
 jobs:
-  format:
+  lint:
     runs-on: ubuntu-latest
-    container: qmkfm/base_container
 
-    # protect against those who develop with their fork on master
-    if: github.repository == 'qmk/qmk_firmware'
+    container: qmkfm/base_container
 
     steps:
     - uses: rlespinasse/[email protected]
 
     - uses: actions/checkout@v2
       with:
-        token: ${{ secrets.API_TOKEN_GITHUB }}
-
-    - name: Install dependencies
-      run: |
-        apt-get update && apt-get install -y dos2unix
+        fetch-depth: 0
 
-    - name: Format files
-      run: |
-        bin/qmk cformat -a
-        bin/qmk pyformat
-        bin/qmk fileformat
+    - uses: trilom/[email protected]
+      id: file_changes
+      with:
+        output: ' '
+        fileOutput: ' '
 
-    - name: Become QMK Bot
+    - name: Run qmk cformat and qmk pyformat
+      shell: 'bash {0}'
       run: |
-        git config user.name 'QMK Bot'
-        git config user.email '[email protected]'
+        qmk cformat -n $(< ~/files.txt)
+        cformat_exit=$?
+        qmk pyformat -n
+        pyformat_exit=$?
 
-    - name: Create Pull Request
-      uses: peter-evans/create-pull-request@v3
-      with:
-        delete-branch: true
-        branch: bugfix/format_${{ env.GITHUB_REF_SLUG }}
-        author: QMK Bot <[email protected]>
-        committer: QMK Bot <[email protected]>
-        commit-message: Format code according to conventions
-        title: '[CI] Format code according to conventions'
+        exit $((cformat_exit + pyformat_exit))

lib/python/qmk/cli/cformat.py

@@ -1,6 +1,7 @@
 """Format C code according to QMK's style.
 """
 import subprocess
+from os import path
 from shutil import which
 
 from argcomplete.completers import FilesCompleter
@@ -9,58 +10,118 @@
 from qmk.path import normpath
 from qmk.c_parse import c_source_files
 
+c_file_suffixes = ('c', 'h', 'cpp')
+core_dirs = ('drivers', 'quantum', 'tests', 'tmk_core', 'platforms')
+ignored = ('tmk_core/protocol/usb_hid', 'quantum/template', 'platforms/chibios')
 
-def cformat_run(files, all_files):
-    """Spawn clang-format subprocess with proper arguments
+
+def find_clang_format():
+    """Returns the path to clang-format.
     """
-    # Determine which version of clang-format to use
-    clang_format = ['clang-format', '-i']
     for clang_version in range(20, 6, -1):
-        binary = 'clang-format-%d' % clang_version
+        binary = f'clang-format-{clang_version}'
+
         if which(binary):
-            clang_format[0] = binary
-            break
+            return binary
+
+    return 'clang-format'
+
+
+def find_diffs(files):
+    """Run clang-format and diff it against a file.
+    """
+    found_diffs = False
+
+    for file in files:
+        cli.log.debug('Checking for changes in %s', file)
+        clang_format = subprocess.Popen([find_clang_format(), file], stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
+        diff = cli.run(['diff', '-u', f'--label=a/{file}', f'--label=b/{file}', str(file), '-'], stdin=clang_format.stdout, capture_output=True)
+
+        if diff.returncode != 0:
+            print(diff.stdout)
+            found_diffs = True
+
+    return found_diffs
+
+
+def cformat_run(files):
+    """Spawn clang-format subprocess with proper arguments
+    """
+    # Determine which version of clang-format to use
+    clang_format = [find_clang_format(), '-i']
+
     try:
-        if not files:
-            cli.log.warn('No changes detected. Use "qmk cformat -a" to format all files')
-            return False
-        subprocess.run(clang_format + [file for file in files], check=True)
+        cli.run(clang_format + list(map(str, files)), check=True, capture_output=False)
         cli.log.info('Successfully formatted the C code.')
+        return True
 
-    except subprocess.CalledProcessError:
+    except subprocess.CalledProcessError as e:
         cli.log.error('Error formatting C code!')
+        cli.log.debug('%s exited with returncode %s', e.cmd, e.returncode)
+        cli.log.debug('STDOUT:')
+        cli.log.debug(e.stdout)
+        cli.log.debug('STDERR:')
+        cli.log.debug(e.stderr)
         return False
 
 
-@cli.argument('-a', '--all-files', arg_only=True, action='store_true', help='Format all core files.')
+def filter_files(files):
+    """Yield only files to be formatted and skip the rest
+    """
+    for file in files:
+        if file.name.split('.')[-1] in c_file_suffixes:
+            yield file
+        else:
+            cli.log.debug('Skipping file %s', file)
+
+
+@cli.argument('-n', '--dry-run', arg_only=True, action='store_true', help="Flag only, don't automatically format.")
 @cli.argument('-b', '--base-branch', default='origin/master', help='Branch to compare to diffs to.')
-@cli.argument('files', nargs='*', arg_only=True, completer=FilesCompleter('.c'), help='Filename(s) to format.')
+@cli.argument('-a', '--all-files', arg_only=True, action='store_true', help='Format all core files.')
+@cli.argument('files', nargs='*', arg_only=True, type=normpath, completer=FilesCompleter('.c'), help='Filename(s) to format.')
 @cli.subcommand("Format C code according to QMK's style.", hidden=False if cli.config.user.developer else True)
 def cformat(cli):
     """Format C code according to QMK's style.
     """
-    # Empty array for files
-    files = []
-    # Core directories for formatting
-    core_dirs = ['drivers', 'quantum', 'tests', 'tmk_core', 'platforms']
-    ignores = ['tmk_core/protocol/usb_hid', 'quantum/template', 'platforms/chibios']
     # Find the list of files to format
     if cli.args.files:
-        files.extend(normpath(file) for file in cli.args.files)
+        files = list(filter_files(cli.args.files))
+
+        if not files:
+            cli.log.error('No C files in filelist: %s', ', '.join(map(str, cli.args.files)))
+            exit(0)
+
         if cli.args.all_files:
             cli.log.warning('Filenames passed with -a, only formatting: %s', ','.join(map(str, files)))
-    # If -a is specified
+
     elif cli.args.all_files:
         all_files = c_source_files(core_dirs)
         # The following statement checks each file to see if the file path is in the ignored directories.
-        files.extend(file for file in all_files if not any(i in str(file) for i in ignores))
-    # No files specified & no -a flag
+        files = [file for file in all_files if not any(i in str(file) for i in ignored)]
+
     else:
-        base_args = ['git', 'diff', '--name-only', cli.args.base_branch]
-        out = subprocess.run(base_args + core_dirs, check=True, stdout=subprocess.PIPE)
-        changed_files = filter(None, out.stdout.decode('UTF-8').split('\n'))
-        filtered_files = [normpath(file) for file in changed_files if not any(i in file for i in ignores)]
-        files.extend(file for file in filtered_files if file.exists() and file.suffix in ['.c', '.h', '.cpp'])
+        git_diff_cmd = ['git', 'diff', '--name-only', cli.args.base_branch, *core_dirs]
+        git_diff = cli.run(git_diff_cmd)
+
+        if git_diff.returncode != 0:
+            cli.log.error("Error running %s", git_diff_cmd)
+            print(git_diff.stderr)
+            return git_diff.returncode
+
+        files = []
+
+        for file in git_diff.stdout.strip().split('\n'):
+            if not any([file.startswith(ignore) for ignore in ignored]):
+                if path.exists(file) and file.split('.')[-1] in c_file_suffixes:
+                    files.append(file)
+
+    # Sanity check
+    if not files:
+        cli.log.error('No changed files detected. Use "qmk cformat -a" to format all files')
+        return False
 
     # Run clang-format on the files we've found
-    cformat_run(files, cli.args.all_files)
+    if cli.args.dry_run:
+        return not find_diffs(files)
+    else:
+        return cformat_run(files)

lib/python/qmk/cli/generate/api.py

@@ -13,6 +13,7 @@
 from qmk.keyboard import list_keyboards
 
 
+@cli.argument('-n', '--dry-run', arg_only=True, action='store_true', help="Don't write the data to disk.")
 @cli.subcommand('Creates a new keymap for the keyboard of your choosing', hidden=False if cli.config.user.developer else True)
 def generate_api(cli):
     """Generates the QMK API data.
@@ -40,10 +41,14 @@ def generate_api(cli):
         keyboard_readme_src = Path('keyboards') / keyboard_name / 'readme.md'
 
         keyboard_dir.mkdir(parents=True, exist_ok=True)
-        keyboard_info.write_text(json.dumps({'last_updated': current_datetime(), 'keyboards': {keyboard_name: kb_all[keyboard_name]}}))
+        keyboard_json = json.dumps({'last_updated': current_datetime(), 'keyboards': {keyboard_name: kb_all[keyboard_name]}})
+        if not cli.args.dry_run:
+            keyboard_info.write_text(keyboard_json)
+            cli.log.debug('Wrote file %s', keyboard_info)
 
-        if keyboard_readme_src.exists():
-            copyfile(keyboard_readme_src, keyboard_readme)
+            if keyboard_readme_src.exists():
+                copyfile(keyboard_readme_src, keyboard_readme)
+                cli.log.debug('Copied %s -> %s', keyboard_readme_src, keyboard_readme)
 
         if 'usb' in kb_all[keyboard_name]:
             usb = kb_all[keyboard_name]['usb']
@@ -57,20 +62,26 @@ def generate_api(cli):
             if 'vid' in usb and 'pid' in usb:
                 usb_list[usb['vid']][usb['pid']][keyboard_name] = usb
 
-    # Write the global JSON files
-    keyboard_all_file.write_text(json.dumps({'last_updated': current_datetime(), 'keyboards': kb_all}, cls=InfoJSONEncoder))
-    usb_file.write_text(json.dumps({'last_updated': current_datetime(), 'usb': usb_list}, cls=InfoJSONEncoder))
-
+    # Generate data for the global files
     keyboard_list = sorted(kb_all)
-    keyboard_list_file.write_text(json.dumps({'last_updated': current_datetime(), 'keyboards': keyboard_list}, cls=InfoJSONEncoder))
-
     keyboard_aliases = json_load(Path('data/mappings/keyboard_aliases.json'))
-    keyboard_aliases_file.write_text(json.dumps({'last_updated': current_datetime(), 'keyboard_aliases': keyboard_aliases}, cls=InfoJSONEncoder))
-
     keyboard_metadata = {
         'last_updated': current_datetime(),
         'keyboards': keyboard_list,
         'keyboard_aliases': keyboard_aliases,
         'usb': usb_list,
     }
-    keyboard_metadata_file.write_text(json.dumps(keyboard_metadata, cls=InfoJSONEncoder))
+
+    # Write the global JSON files
+    keyboard_all_json = json.dumps({'last_updated': current_datetime(), 'keyboards': kb_all}, cls=InfoJSONEncoder)
+    usb_json = json.dumps({'last_updated': current_datetime(), 'usb': usb_list}, cls=InfoJSONEncoder)
+    keyboard_list_json = json.dumps({'last_updated': current_datetime(), 'keyboards': keyboard_list}, cls=InfoJSONEncoder)
+    keyboard_aliases_json = json.dumps({'last_updated': current_datetime(), 'keyboard_aliases': keyboard_aliases}, cls=InfoJSONEncoder)
+    keyboard_metadata_json = json.dumps(keyboard_metadata, cls=InfoJSONEncoder)
+
+    if not cli.args.dry_run:
+        keyboard_all_file.write_text(keyboard_all_json)
+        usb_file.write_text(usb_json)
+        keyboard_list_file.write_text(keyboard_list_json)
+        keyboard_aliases_file.write_text(keyboard_aliases_json)
+        keyboard_metadata_file.write_text(keyboard_metadata_json)

lib/python/qmk/cli/pyformat.py

@@ -5,13 +5,22 @@
 import subprocess
 
 
+@cli.argument('-n', '--dry-run', arg_only=True, action='store_true', help="Flag only, don't automatically format.")
 @cli.subcommand("Format python code according to QMK's style.", hidden=False if cli.config.user.developer else True)
 def pyformat(cli):
     """Format python code according to QMK's style.
     """
+    edit = '--diff' if cli.args.dry_run else '--in-place'
+    yapf_cmd = ['yapf', '-vv', '--recursive', edit, 'bin/qmk', 'lib/python']
     try:
-        subprocess.run(['yapf', '-vv', '-ri', 'bin/qmk', 'lib/python'], check=True)
-        cli.log.info('Successfully formatted the python code in `bin/qmk` and `lib/python`.')
+        cli.run(yapf_cmd, check=True, capture_output=False)
+        cli.log.info('Python code in `bin/qmk` and `lib/python` is correctly formatted.')
+        return True
 
     except subprocess.CalledProcessError:
-        cli.log.error('Error formatting python code!')
+        if cli.args.dry_run:
+            cli.log.error('Python code in `bin/qmk` and `lib/python` incorrectly formatted!')
+        else:
+            cli.log.error('Error formatting python code!')
+
+    return False

lib/python/qmk/cli/pytest.py

@@ -11,6 +11,7 @@
 def pytest(cli):
     """Run several linting/testing commands.
     """
-    flake8 = subprocess.run(['flake8', 'lib/python', 'bin/qmk'])
     nose2 = subprocess.run(['nose2', '-v'])
+    flake8 = subprocess.run(['flake8', 'lib/python', 'bin/qmk'])
+
     return flake8.returncode | nose2.returncode

lib/python/qmk/tests/test_cli_commands.py

@@ -33,10 +33,15 @@ def check_returncode(result, expected=[0]):
 
 
 def test_cformat():
-    result = check_subcommand('cformat', 'quantum/matrix.c')
+    result = check_subcommand('cformat', '-n', 'quantum/matrix.c')
     check_returncode(result)
 
 
+def test_cformat_all():
+    result = check_subcommand('cformat', '-n', '-a')
+    check_returncode(result, [0, 1])
+
+
 def test_compile():
     result = check_subcommand('compile', '-kb', 'handwired/pytest/basic', '-km', 'default', '-n')
     check_returncode(result)
@@ -83,9 +88,9 @@ def test_hello():
 
 
 def test_pyformat():
-    result = check_subcommand('pyformat')
+    result = check_subcommand('pyformat', '--dry-run')
     check_returncode(result)
-    assert 'Successfully formatted the python code' in result.stdout
+    assert 'Python code in `bin/qmk` and `lib/python` is correctly formatted.' in result.stdout
 
 
 def test_list_keyboards():
@@ -225,6 +230,11 @@ def test_clean():
     assert result.stdout.count('done') == 2
 
 
+def test_generate_api():
+    result = check_subcommand('generate-api', '--dry-run')
+    check_returncode(result)
+
+
 def test_generate_rgb_breathe_table():
     result = check_subcommand("generate-rgb-breathe-table", "-c", "1.2", "-m", "127")
     check_returncode(result)