Fixed a recompilation issue in CI, tweaked coverage.py a bit more

This was lost in the Travis -> GitHub transition, in serializing some of
the jobs, I missed that we need to clean between tests with different
geometry configurations. Otherwise we end up running outdated binaries,
which explains some of the weird test behavior we were seeing.

Also tweaked a few script things:
- Better subprocess error reporting (dump stderr on failure)
- Fixed a BUILDDIR rule issue in test.py
- Changed test-not-run status to None instead of undefined

.github/workflows/test.yml

@@ -13,8 +13,6 @@ jobs:
       fail-fast: false
       matrix:
         arch: [x86_64, thumb, mips, powerpc]
-    env:
-      TESTFLAGS: --coverage
 
     steps:
       - uses: actions/checkout@v2
@@ -24,6 +22,13 @@ jobs:
           sudo apt-get update -qq
           sudo apt-get install -qq python3 python3-pip lcov
           sudo pip3 install toml
+          gcc --version
+
+          # collect coverage
+          mkdir -p coverage
+          echo "TESTFLAGS=$TESTFLAGS --coverage=`
+            `coverage/${{github.job}}-${{matrix.arch}}.info" >> $GITHUB_ENV
+
       # cross-compile with ARM Thumb (32-bit, little-endian)
       - name: install-thumb
         if: matrix.arch == 'thumb'
@@ -60,7 +65,7 @@ jobs:
           echo "EXEC=qemu-ppc" >> $GITHUB_ENV
           powerpc-linux-gnu-gcc --version
           qemu-ppc -version
-      # test configurations
+
       # make sure example can at least compile
       - name: test-example
         run: |
@@ -71,45 +76,65 @@ jobs:
             -Duser_provided_block_device_erase=NULL \
             -Duser_provided_block_device_sync=NULL \
             -include stdio.h"
+
+      # test configurations
       # normal+reentrant tests
       - name: test-default
-        run: make test_dirs TESTFLAGS+="-nrk"
+        run: |
+          make clean
+          make test TESTFLAGS+="-nrk"
       # NOR flash: read/prog = 1 block = 4KiB
       - name: test-nor
-        run: make test TESTFLAGS+="-nrk
+        run: |
-          -DLFS_READ_SIZE=1 -DLFS_BLOCK_SIZE=4096" VERBOSE=1
+          make clean
+          make test TESTFLAGS+="-nrk \
+            -DLFS_READ_SIZE=1 -DLFS_BLOCK_SIZE=4096"
       # SD/eMMC: read/prog = 512 block = 512
       - name: test-emmc
-        run: make test TESTFLAGS+="-nrk
+        run: |
-          -DLFS_READ_SIZE=512 -DLFS_BLOCK_SIZE=512"
+          make clean
+          make test TESTFLAGS+="-nrk \
+            -DLFS_READ_SIZE=512 -DLFS_BLOCK_SIZE=512"
       # NAND flash: read/prog = 4KiB block = 32KiB
       - name: test-nand
-        run: make test TESTFLAGS+="-nrk
+        run: |
-          -DLFS_READ_SIZE=4096 -DLFS_BLOCK_SIZE=\(32*1024\)"
+          make clean
+          make test TESTFLAGS+="-nrk \
+            -DLFS_READ_SIZE=4096 -DLFS_BLOCK_SIZE=\(32*1024\)"
       # other extreme geometries that are useful for various corner cases
       - name: test-no-intrinsics
-        run: make test TESTFLAGS+="-nrk
+        run: |
-          -DLFS_NO_INTRINSICS"
+          make clean
+          make test TESTFLAGS+="-nrk \
+            -DLFS_NO_INTRINSICS"
       - name: test-byte-writes
-        run: make test TESTFLAGS+="-nrk
+        # it just takes too long to test byte-level writes when in qemu,
-          -DLFS_READ_SIZE=1 -DLFS_CACHE_SIZE=1"
+        # should be plenty covered by the other configurations
+        if: matrix.arch == 'x86_64'
+        run: |
+          make clean
+          make test TESTFLAGS+="-nrk \
+            -DLFS_READ_SIZE=1 -DLFS_CACHE_SIZE=1"
       - name: test-block-cycles
-        run: make test TESTFLAGS+="-nrk
+        run: |
-          -DLFS_BLOCK_CYCLES=1"
+          make clean
+          make test TESTFLAGS+="-nrk \
+            -DLFS_BLOCK_CYCLES=1"
       - name: test-odd-block-count
-        run: make test TESTFLAGS+="-nrk
+        run: |
-          -DLFS_BLOCK_COUNT=1023 -DLFS_LOOKAHEAD_SIZE=256"
+          make clean
+          make test TESTFLAGS+="-nrk \
+            -DLFS_BLOCK_COUNT=1023 -DLFS_LOOKAHEAD_SIZE=256"
       - name: test-odd-block-size
-        run: make test TESTFLAGS+="-nrk
+        run: |
-          -DLFS_READ_SIZE=11 -DLFS_BLOCK_SIZE=704"
+          make clean
+          make test TESTFLAGS+="-nrk \
+            -DLFS_READ_SIZE=11 -DLFS_BLOCK_SIZE=704"
 
       # collect coverage
       - name: collect-coverage
         continue-on-error: true
         run: |
-          mkdir -p coverage
-          lcov $(for f in tests/*.toml.cumul.info ; do echo "-a $f" ; done) \
-            -o coverage/${{github.job}}-${{matrix.arch}}.info
           # we only care about littlefs's actual source
           lcov -e coverage/${{github.job}}-${{matrix.arch}}.info \
             $(for f in lfs*.c ; do echo "/$f" ; done) \
@@ -127,10 +152,10 @@ jobs:
         continue-on-error: true
         run: |
           mkdir -p results
-          # TODO remove the need for OBJ
+          # TODO remove the need for SRC
           make clean
           make code \
-            OBJ="$(echo lfs*.c | sed 's/\.c/\.o/g')" \
+            SRC="$(echo lfs*.c)" \
             CFLAGS+=" \
               -DLFS_NO_ASSERT \
               -DLFS_NO_DEBUG \
@@ -143,7 +168,7 @@ jobs:
           mkdir -p results
           make clean
           make code \
-            OBJ="$(echo lfs*.c | sed 's/\.c/\.o/g')" \
+            SRC="$(echo lfs*.c)" \
             CFLAGS+=" \
               -DLFS_NO_ASSERT \
               -DLFS_NO_DEBUG \
@@ -157,7 +182,7 @@ jobs:
           mkdir -p results
           make clean
           make code \
-            OBJ="$(echo lfs*.c | sed 's/\.c/\.o/g')" \
+            SRC="$(echo lfs*.c)" \
             CFLAGS+=" \
               -DLFS_NO_ASSERT \
               -DLFS_NO_DEBUG \
@@ -171,7 +196,7 @@ jobs:
           mkdir -p results
           make clean
           make code \
-            OBJ="$(echo lfs*.c | sed 's/\.c/\.o/g')" \
+            SRC="$(echo lfs*.c)" \
             CFLAGS+=" \
               -DLFS_NO_ASSERT \
               -DLFS_NO_DEBUG \

Makefile

@@ -16,6 +16,7 @@ else
 TARGET ?= $(BUILDDIR)lfs.a
 endif
 
+
 CC ?= gcc
 AR ?= ar
 SIZE ?= size
@@ -24,14 +25,9 @@ NM ?= nm
 LCOV ?= lcov
 
 SRC ?= $(wildcard *.c bd/*.c)
-OBJ := $(SRC:%.c=%.o)
+OBJ := $(SRC:%.c=$(BUILDDIR)%.o)
-DEP := $(SRC:%.c=%.d)
+DEP := $(SRC:%.c=$(BUILDDIR)%.d)
-ASM := $(SRC:%.c=%.s)
+ASM := $(SRC:%.c=$(BUILDDIR)%.s)
-ifdef BUILDDIR
-override OBJ := $(addprefix $(BUILDDIR),$(OBJ))
-override DEP := $(addprefix $(BUILDDIR),$(DEP))
-override ASM := $(addprefix $(BUILDDIR),$(ASM))
-endif
 
 ifdef DEBUG
 override CFLAGS += -O0 -g3
@@ -81,10 +77,6 @@ tags:
 code: $(OBJ)
 	./scripts/code.py $^ $(CODEFLAGS)
 
-.PHONY: coverage
-coverage:
-	./scripts/coverage.py $(BUILDDIR)tests/*.toml.info $(COVERAGEFLAGS)
-
 .PHONY: test
 test:
 	./scripts/test.py $(TESTFLAGS)
@@ -92,6 +84,10 @@ test:
 test%: tests/test$$(firstword $$(subst \#, ,%)).toml
 	./scripts/test.py $@ $(TESTFLAGS)
 
+.PHONY: coverage
+coverage:
+	./scripts/coverage.py $(BUILDDIR)tests/*.toml.info $(COVERAGEFLAGS)
+
 # rules
 -include $(DEP)
 .SUFFIXES:

scripts/code.py

@@ -28,11 +28,20 @@ def collect(paths, **args):
         cmd = args['nm_tool'] + ['--size-sort', path]
         if args.get('verbose'):
             print(' '.join(shlex.quote(c) for c in cmd))
-        proc = sp.Popen(cmd, stdout=sp.PIPE, universal_newlines=True)
+        proc = sp.Popen(cmd,
+            stdout=sp.PIPE,
+            stderr=sp.PIPE if not args.get('verbose') else None,
+            universal_newlines=True)
         for line in proc.stdout:
             m = pattern.match(line)
             if m:
                 results[(path, m.group('func'))] += int(m.group('size'), 16)
+        proc.wait()
+        if proc.returncode != 0:
+            if not args.get('verbose'):
+                for line in proc.stderr:
+                    sys.stdout.write(line)
+            sys.exit(-1)
 
     flat_results = []
     for (file, func), size in results.items():

scripts/test.py

@@ -34,14 +34,18 @@ $(foreach target,$(SRC),$(eval $(FLATTEN)))
 %(path)s.test: %(path)s.test.o \\
         $(foreach t,$(subst /,.,$(SRC:.c=.o)),%(path)s.$t)
     $(CC) $(CFLAGS) $^ $(LFLAGS) -o $@
+
+# needed in case builddir is different
+%(path)s%%.o: %(path)s%%.c
+    $(CC) -c -MMD $(CFLAGS) $< -o $@
 """
 COVERAGE_RULES = """
 %(path)s.test: override CFLAGS += -fprofile-arcs -ftest-coverage
 
 # delete lingering coverage
-%(path)s.test: | %(path)s.clean
+%(path)s.test: | %(path)s.info.clean
-.PHONY: %(path)s.clean
+.PHONY: %(path)s.info.clean
-%(path)s.clean:
+%(path)s.info.clean:
     rm -f %(path)s*.gcda
 
 # accumulate coverage info
@@ -52,10 +56,11 @@ COVERAGE_RULES = """
         --rc 'geninfo_adjust_src_path=$(shell pwd)' \\
         -o $@)
     $(LCOV) -e $@ $(addprefix /,$(SRC)) -o $@
-
+ifdef COVERAGETARGET
-.PHONY: %(path)s.cumul.info
+    $(strip $(LCOV) -a $@ \\
-%(path)s.cumul.info: %(path)s.info
+        $(addprefix -a ,$(wildcard $(COVERAGETARGET))) \\
-    $(LCOV) -a $< $(addprefix -a ,$(wildcard $@)) -o $@
+        -o $(COVERAGETARGET))
+endif
 """
 GLOBALS = """
 //////////////// AUTOGENERATED TEST ////////////////
@@ -142,6 +147,8 @@ class TestCase:
         self.if_ = config.get('if', None)
         self.in_ = config.get('in', None)
 
+        self.result = None
+
     def __str__(self):
         if hasattr(self, 'permno'):
             if any(k not in self.case.defines for k in self.defines):
@@ -202,7 +209,7 @@ class TestCase:
                 len(self.filter) >= 2 and
                 self.filter[1] != self.permno):
             return False
-        elif args.get('no_internal', False) and self.in_ is not None:
+        elif args.get('no_internal') and self.in_ is not None:
             return False
         elif self.if_ is not None:
             if_ = self.if_
@@ -236,7 +243,7 @@ class TestCase:
                 try:
                     with open(disk, 'w') as f:
                         f.truncate(0)
-                    if args.get('verbose', False):
+                    if args.get('verbose'):
                         print('truncate --size=0', disk)
                 except FileNotFoundError:
                     pass
@@ -260,14 +267,14 @@ class TestCase:
                     '-ex', 'r'])
             ncmd.extend(['--args'] + cmd)
 
-            if args.get('verbose', False):
+            if args.get('verbose'):
                 print(' '.join(shlex.quote(c) for c in ncmd))
             signal.signal(signal.SIGINT, signal.SIG_IGN)
             sys.exit(sp.call(ncmd))
 
         # run test case!
         mpty, spty = pty.openpty()
-        if args.get('verbose', False):
+        if args.get('verbose'):
             print(' '.join(shlex.quote(c) for c in cmd))
         proc = sp.Popen(cmd, stdout=spty, stderr=spty)
         os.close(spty)
@@ -283,7 +290,7 @@ class TestCase:
                         break
                     raise
                 stdout.append(line)
-                if args.get('verbose', False):
+                if args.get('verbose'):
                     sys.stdout.write(line)
                 # intercept asserts
                 m = re.match(
@@ -322,7 +329,7 @@ class ValgrindTestCase(TestCase):
         return not self.leaky and super().shouldtest(**args)
 
     def test(self, exec=[], **args):
-        verbose = args.get('verbose', False)
+        verbose = args.get('verbose')
         uninit = (self.defines.get('LFS_ERASE_VALUE', None) == -1)
         exec = [
             'valgrind',
@@ -548,7 +555,7 @@ class TestSuite:
             mk.write('\n')
 
             # add coverage hooks?
-            if args.get('coverage', False):
+            if args.get('coverage'):
                 mk.write(COVERAGE_RULES.replace(4*' ', '\t') % dict(
                     path=self.path))
                 mk.write('\n')
@@ -593,7 +600,7 @@ class TestSuite:
                 if not args.get('verbose', True):
                     sys.stdout.write(FAIL)
                     sys.stdout.flush()
-                if not args.get('keep_going', False):
+                if not args.get('keep_going'):
                     if not args.get('verbose', True):
                         sys.stdout.write('\n')
                     raise
@@ -615,11 +622,11 @@ def main(**args):
 
     # and what class of TestCase to run
     classes = []
-    if args.get('normal', False):
+    if args.get('normal'):
         classes.append(TestCase)
-    if args.get('reentrant', False):
+    if args.get('reentrant'):
         classes.append(ReentrantTestCase)
-    if args.get('valgrind', False):
+    if args.get('valgrind'):
         classes.append(ValgrindTestCase)
     if not classes:
         classes = [TestCase]
@@ -664,7 +671,7 @@ def main(**args):
         list(it.chain.from_iterable(['-f', m] for m in makefiles)) +
         [target for target in targets])
     mpty, spty = pty.openpty()
-    if args.get('verbose', False):
+    if args.get('verbose'):
         print(' '.join(shlex.quote(c) for c in cmd))
     proc = sp.Popen(cmd, stdout=spty, stderr=spty)
     os.close(spty)
@@ -678,14 +685,14 @@ def main(**args):
                 break
             raise
         stdout.append(line)
-        if args.get('verbose', False):
+        if args.get('verbose'):
             sys.stdout.write(line)
         # intercept warnings
         m = re.match(
             '^{0}([^:]+):(\d+):(?:\d+:)?{0}{1}:{0}(.*)$'
             .format('(?:\033\[[\d;]*.| )*', 'warning'),
             line)
-        if m and not args.get('verbose', False):
+        if m and not args.get('verbose'):
             try:
                 with open(m.group(1)) as f:
                     lineno = int(m.group(2))
@@ -698,9 +705,8 @@ def main(**args):
             except:
                 pass
     proc.wait()
-
     if proc.returncode != 0:
-        if not args.get('verbose', False):
+        if not args.get('verbose'):
             for line in stdout:
                 sys.stdout.write(line)
         sys.exit(-1)
@@ -718,7 +724,7 @@ def main(**args):
         print('filtered down to %d permutations' % total)
 
     # only requested to build?
-    if args.get('build', False):
+    if args.get('build'):
         return 0
 
     print('====== testing ======')
@@ -733,12 +739,9 @@ def main(**args):
     failed = 0
     for suite in suites:
         for perm in suite.perms:
-            if not hasattr(perm, 'result'):
-                continue
-
             if perm.result == PASS:
                 passed += 1
-            else:
+            elif isinstance(perm.result, TestFailure):
                 sys.stdout.write(
                     "\033[01m{path}:{lineno}:\033[01;31mfailure:\033[m "
                     "{perm} failed\n".format(
@@ -759,25 +762,33 @@ def main(**args):
                 sys.stdout.write('\n')
                 failed += 1
 
-    if args.get('coverage', False):
+    if args.get('coverage'):
         # collect coverage info
-        # why -j1? lcov doesn't work in parallel because of gcov issues
+        # why -j1? lcov doesn't work in parallel because of gcov limitations
         cmd = (['make', '-j1', '-f', 'Makefile'] +
             list(it.chain.from_iterable(['-f', m] for m in makefiles)) +
-            [re.sub('\.test$', '.cumul.info', target) for target in targets])
+            (['COVERAGETARGET=%s' % args['coverage']]
-        if args.get('verbose', False):
+                if isinstance(args['coverage'], str) else []) +
+            [suite.path + '.info' for suite in suites
+                if any(perm.result == PASS for perm in suite.perms)])
+        if args.get('verbose'):
             print(' '.join(shlex.quote(c) for c in cmd))
         proc = sp.Popen(cmd,
-            stdout=sp.DEVNULL if not args.get('verbose', False) else None)
+            stdout=sp.PIPE if not args.get('verbose') else None,
+            stderr=sp.STDOUT if not args.get('verbose') else None,
+            universal_newlines=True)
         proc.wait()
         if proc.returncode != 0:
+            if not args.get('verbose'):
+                for line in proc.stdout:
+                    sys.stdout.write(line)
             sys.exit(-1)
 
-    if args.get('gdb', False):
+    if args.get('gdb'):
         failure = None
         for suite in suites:
             for perm in suite.perms:
-                if getattr(perm, 'result', PASS) != PASS:
+                if isinstance(perm.result, TestFailure):
                     failure = perm.result
         if failure is not None:
             print('======= gdb ======')
@@ -827,11 +838,11 @@ if __name__ == "__main__":
         help="Run tests with another executable prefixed on the command line.")
     parser.add_argument('--disk',
         help="Specify a file to use for persistent/reentrant tests.")
-    parser.add_argument('--coverage', action='store_true',
+    parser.add_argument('--coverage', type=lambda x: x if x else True,
+        nargs='?', const='',
         help="Collect coverage information during testing. This uses lcov/gcov \
-            to accumulate coverage information into *.info files. Note \
+            to accumulate coverage information into *.info files. May also \
-            coverage is not reset between runs, allowing multiple runs to \
+            a path to a *.info file to accumulate coverage info into.")
-            contribute to coverage.")
     parser.add_argument('--build-dir',
         help="Build relative to the specified directory instead of the \
             current directory.")

tests/test_alloc.toml

@@ -485,8 +485,7 @@ code = '''
 [[case]] # split dir test
 define.LFS_BLOCK_SIZE = 512
 define.LFS_BLOCK_COUNT = 1024
-if = 'False'
+if = 'LFS_BLOCK_SIZE == 512 && LFS_BLOCK_COUNT == 1024'
-#if = 'LFS_BLOCK_SIZE == 512 && LFS_BLOCK_COUNT == 1024'
 code = '''
     lfs_format(&lfs, &cfg) => 0;
     lfs_mount(&lfs, &cfg) => 0;
@@ -531,8 +530,7 @@ code = '''
 [[case]] # outdated lookahead test
 define.LFS_BLOCK_SIZE = 512
 define.LFS_BLOCK_COUNT = 1024
-if = 'False'
+if = 'LFS_BLOCK_SIZE == 512 && LFS_BLOCK_COUNT == 1024'
-#if = 'LFS_BLOCK_SIZE == 512 && LFS_BLOCK_COUNT == 1024'
 code = '''
     lfs_format(&lfs, &cfg) => 0;
     lfs_mount(&lfs, &cfg) => 0;
@@ -597,8 +595,7 @@ code = '''
 [[case]] # outdated lookahead and split dir test
 define.LFS_BLOCK_SIZE = 512
 define.LFS_BLOCK_COUNT = 1024
-if = 'False'
+if = 'LFS_BLOCK_SIZE == 512 && LFS_BLOCK_COUNT == 1024'
-#if = 'LFS_BLOCK_SIZE == 512 && LFS_BLOCK_COUNT == 1024'
 code = '''
     lfs_format(&lfs, &cfg) => 0;
     lfs_mount(&lfs, &cfg) => 0;