Testing scripts are too different, and code quality is inconsistent

[dwRchyngqxs]

I think there are two issues that we kind of swept under the rug and they came to bite us now in the form of inconsistent marking.
We have maintained the build+testing+marking processes separately and overall poorly.

I propose several fixes going forward; EDIT: I guess this has become a meta-issue / discussion so let's just embrace it and also list item raised by other people here:

• Make a base marking repo for the marking infrastructure.
• Have the coursework repo be a submodule of the base marking repo.
• Share provided tests.
• Disable ASAN recovery.
• Update dependencies.
• Update CI image.
• Make PR/issue tags to know whether they are to be merged before marking or for the following year.
• Make a list of interesting compilers to test changes on.
• Justify the benefit of several compilation flows.
• Move toward a unified building+testing+marking again.
• Move the Rights and Components part of this issue to a .md file in the marking repo.

No longer to do:

• Make a project listing all the changes to be done for a given year, and all the long running issues.
• Make a script+CI to ensure building and report are consistent between all the different scripts scattered. (UI can stay different.) Not necessary: the TA script will import the student script.
• Have a way to test compilers in different machine and with different environments. People seem against me on that one. Since github doesn't provide distros other than ubuntu for CI, I guess I can just try this item myself using act.
• Improve code quality for building+testing+marking scripts. Tackled progressively with the recent PRs. Improvements are still welcomed.

About this last point: These build scripts are code that students may wish to read and understand, so we should strive to keep them readable, understandable, and using best code practices.
Thank you @Fiwo735 for the amazing work. Global variables are mostly gone, format strings are used instead of + concatenation. Most of the ugly code is gone! I'm still not satisfied with our class use but I will take the time to open issues progressively.

Components:

• A repo for the marking infrastructure with a marking script, hidden tests, a Dockerfile, the marking CI, a test validation CI, and the coursework as a submodule.
• For each year a marking repo with student marks and feedback.
• A prebuilt riscv-gcc+spike+pk as an artifact on langproc-cw.
• Coursework repo with a skeleton compiler, provided tests, course documentation, debugging documentation, a Dockerfile, 2 building scripts, a testing script. -> Ideally the build workflow is unified.
• A Coursework repo for each group with CI permission (thanks @Fiwo735 @Jpnock).
• The CI marking Docker image.

[Fiwo735]

1. Base marking repo with PRs each year - agreed, I don't see a point of a completely new repo each year.
2. Base marking repo containing langproc-cw as submodule - agreed.
3. Project listing all the changes - I am not sure I see the point, how would that be better than issues/pull requests which we try to address before realising the cw? I also believe the cw is maturing, so wouldn't be surprised if the changes in the coming years are less frequent.
4. Script+CI - I think we already have CI in both cw and marking repos, so I assume moving forward we'd keep the cw CI and update marking CI to rely/acknowledge cw once it is a submodule?
5. Interesting compilers to test changes on - would it be a stretch to test big changes on e.g. all last year's compilers? Regardless, what would we be actually testing - that the number of passing test cases have not been changed after a big change?
6. Test compilers in different machine and with different environments - I don't think that is needed given we rely on the Docker environment, right?
7. Review the code for the building+testing+marking collectively - I think once we have base marking repo which instantiates cw repo as submodule, that should happen naturally
8. Inconsistencies in the coding style within the same file as well as code lines which hurt - I think the best way to fix them would be to create issues regarding such files (or directly pull requests if you feel like it). I agree we should be valuing high quality code to serve as example for students.
9. Permissions - definitely the artifact should be handled better. As for who has permissions, I still have mine from previous years, so I think I can extend write (=push) permissions to anyone interested, as I don't think most TAs each year end up needing to push. On that topic, just extended the permissions to you, as I wasn't aware you didn't have them before.

[dwRchyngqxs]

3. My idea was put all the stuff we want to have get done before the student start working on coursework somewhere so that we know what is to be done and what has already been done.
4. Yes, in particular a CI to test consistency between TA_test.sh and script/test.py.
5. I don't think it's worth spending that much time testing and remembering which compiler did or did not compile; a short list would be fine. Alternatively an easy way to test on all compilers from a specific year would also work.
   We could test that our changes do not break compiler which build successfully, be able to discuss breaking changes if detected, and assess consistency of marking.
6. Last year we got marking reliability issues. We don't necessarily need that but it would be nice to improve portability so that we can get away from being forced to use a fixed Docker that may break on update. Docker is a symptomatic treatment, ideally it should not be needed.
7. Thank you.

[lemon-gith]

3 - I know that this is the style adopted by the Linux kernel development and Rust compiler teams, but we're not managing an open-source project, nor anything on that scale. For most projects, the project management offered by GitHub Issues is perfect: what is to be done remains as a raised issue, what has been done is merged or closed. In my opinion, that workflow is more than enough, especially if we make that base repo for marking (which I think is a great idea!)

5 - I actually think it would be quite nice to have a little list of compilers. It would give us a benchmark to run our new tests against, which can allow us to gauge whether or not it's a redundant test, a Sisyphean test, or a good challenge. Possibly just a little compilers.txt where we plop permalinks to some of the best compilers every year?

6 - Even as the one that flagged up those errors, I wholeheartedly disagree with moving away from Docker (unless a new industry standard for portability has dropped recently). I could write a whole essay about why I think Docker is a great choice for this problem, but in short, it's a good thing to teach students, it has a relatively low upfront cost and usage overhead (vs a VM), it has good support and documentation, and it's relatively simple and convenient.

8 - Agreed, if you feel pain deep in your heart, create an issue, and fix it (unless it's medical, then go to the hospital...), classic SWE workflow 😂 I'll try to keep that in mind for any code changes I make, too.

[saturn691]

6. I think for testing we should just use the provided scripts/test.py --use_cmake with the hidden test cases in the given Docker container. The script will pick up all test cases as long as the naming is consistent enough. There's no need for additional scripts.

[Jpnock]

A few thoughts / questions from my side:

• On Docker: I agree it can feel a bit slow/clunky at times, but I think it still brings real benefits. It standardises setup across machines, reduces the support surface area to "Docker + our image", and makes it straightforward to ship fixes (like the pk patch last year) via a single rebuild command. I also don’t see the industry moving away from OCI images anytime soon (even if containerd is often the common runtime underneath), especially given how much tooling in AI/devops is built around them now. Side note: if you're on Mac, I'd strongly recommend trying out OrbStack; it's a night and day difference compared to Docker Desktop imo.

• On the pk bug: I note that the pk bug fix was only made to the marking Docker image last year without changing the coursework Dockerfile. If I recall correctly, the bugfix appeared to change the marks on some compilers (mostly negligible, i.e. +/- 1 mark, however for one compiler I vaguely remember the fix significantly reduced! the marks awarded), so this does need investigating for this year and the main repo needs updating. I believe the solution last year was to mark with and without the pk bug fix and take the highest mark as to not disadvantage anyone.

• ASAN recovery: I’m a bit unsure about disabling halting on ASAN violations (e.g. -fsanitize-recover=address / halt_on_error=0). The main upside is you might catch more issues in one run, but the tradeoff is that follow-on reports can be misleading because they’re triggered after the first corruption. In practice that can increase noise rather than giving one clear stack trace to fix. What’s the main motivation here? As Google puts it in their old sanitizer wiki "WARNING: USE AT YOUR OWN RISK". There's maybe an argument for using these settings in the marking scripts but I think there's little point in recovering and potentially having many passing tests that then fail during marking just because the UB acts somewhat different on the marking machine. If these issues have to be actively solved by students, it probably will save them time too (since they don't have a choice to ignore UB which could be mysteriously breaking some of their other tests).

• LeakSanitizer: Related to the above: do we need leak detection enabled by default? From previous years, I remember a lot of leak reports coming from the parser/lexer, and I’m not sure leak-free C++ is a primary learning goal for the coursework. Since the compiler runs for only a few seconds, leaks are likely negligible in practice, and enabling LSAN by default may add more noise than signal. Unless things have recently improved, my takeaway from the discussion in langproc-cw issue Memory leaks in compiler skeleton #21 was to avoid LSAN by default and use valgrind when you specifically want to investigate leaks (cc @simon-staal). Perhaps we could ship valgrind in the Docker image and expose it behind an optional flag; that would also align with needing -g and -O0 for useful line-number output (as per the docs).

• Defaulting to -O3: I’m also a bit hesitant about -O3 as the default. -g -O0 made stack traces and debugging much easier (due to the emission of accurate code line-numbers), and it also reduces the chance that optimisation amplifies UB in confusing ways. Do we have a sense of the actual performance win we’re getting here, and whether it’s large enough to justify the debugging cost? Why not just default to running multiple instances of the compiler in parallel in the test script instead? We'd also want to still run in -O0 during marking to minimise the effect UB has, so this should be the default setting IMO.

• Complexity / maintenance: Stepping back, we now have three test scripts and two build systems. I worry this is starting to get harder to reason about and maintain. The old bash script wasn’t pretty, but it was short and very explicit about what commands ran. The Python script has a lot of nice features, but at ~500+ lines it’s harder to be confident that all edge cases are handled. I'm not sure about the best approach for this. The coursework spec expects a working Makefile to be submitted, and it should stay this way to reduce marking complexity -- we need to be careful that students don't get the impression they can delete this or only rely on cmake.

Side note: from a quick skim of the testing file, I see the return_code of the cmake configure step is ignored. So there's potentially cases where cmake build gets called on a stale set of makefiles if configure fails.

langproc-cw/scripts/test.py

Lines 362 to 372 in 457ea7e

	return_code, error_msg, _ = run_subprocess(
	cmd=["cmake", "-S", PROJECT_LOCATION, "-B", BUILD_FOLDER, "-DCMAKE_BUILD_TYPE=Release"],
	timeout=BUILD_TIMEOUT_SECONDS,
	silent=silent
	)
	return_code, error_msg, _ = run_subprocess(
	cmd=["cmake", "--build", BUILD_FOLDER], timeout=BUILD_TIMEOUT_SECONDS, silent=silent
	)
	if return_code != 0:
	print(RED + "Error when running cmake:", error_msg + RESET)
	return False

• CI: we should make sure CI is working from the start this year. When the coursework repos are created, each repo needs to be added to the Actions permissions in order to pull the image. See here (cc @Fiwo735 you should have access to this too): https://github.com/orgs/LangProc/packages/container/langproc-2022-cw%2Fenvironment/settings

• Marking CI: one of the better iterations of the CI marking actions workflow is here, but it currently uploads results to a (likely defunct) Cloudflare Worker; instead, we should commit the full marking run outputs (incl. logs) to a per-team branch (e.g. marking-2025-team2) and gather results by writing a script to pulling all the branches and parse a new results.csv on that marking branch. As a plus, this gives us a permanent record of the marking script, flags, results, and full compiler outputs (currently GitHub actions logs get automatically wiped after a short while). If we did this, maybe it'd be good to keep the per year marking repo.

[dwRchyngqxs]

• On projects: ok, I guess I'll scrap this idea. If we encounter the situation of not knowing which PR should be merged this year vs next year when we inevitably make changes in the middle of the coursework, we can just use PR tags.
• On Docker: It's not about speed nor preferring VMs (I don't). It's about Docker preventing people from realising they're writing code specific to the exact Docker environment we use at the time, that is non-portable code that may break on image update, or distro switch, or when tested on an actual machine instead of a container. As far as I know the standard for compilers is for them to be written in a portable way, build on every supported OS (provided dependencies are installed), not in a container, and run natively, not in a container. I'm not saying we should not use Docker, but we should make sure our setup is not so brittle that it working is dependent on Docker.
• ASAN recovery: We may in fact benefit from disabling recovery. I think I enabled it to have more test passing because on student compilers we used exitcode=0. Feel free to disable it in langproc-cw.
• LeakSanitizer: If there are leaks in the skeleton then it is an issue and we should fix it, and disable LSAN until it is fixed. Otherwise it is a very helpful warning for students about wrong code. IIRC if they don't learn memory management in this course they graduate without having done it. Correct me if I'm wrong on that please. If I'm right I think there is a discussion to have.
  IMO forcing the students to think "who owns this piece of memory, who should clean it" gets them part of the way to the though "who is responsible for handling this process of the assembly production". Worst case students ignore the warnings and still have a working compiler.
• Defaulting to -O3: Same opinion as you. I need to see numbers about the performance gain from compiling with clang vs gcc and -O3 vs -O0. Wouldn't it be nice to have a list of student compilers to test that on?
• Complexity / maintenance: Actually, only 2 testing scripts, the student bash script got nuked in a PR. But yeah, same as you. A big part of my grievances about code quality, readability, and consistency were about this python script. Also, I remember encountering in one of my other projects an issue with return code being lost using the Python subprocess module when running several subprocesses at the same time. Bash is mature and doesn't have this kind of issues. So yeah, I disagree with @saturn691. On top of that to specifically address your point, in the marking script there is: more leniency, production of useful supplemental data for marking purposes, anti-cheat, and more tests with a different handling of pass/fail count. I'm not against reusing some parts assuming the student didn't modify their script to cheat.

I agree with the rest of @Jpnock 's points.

[Fiwo735]

• Docker: I think it is the easiest solution by far and on that topic, please see pull request Updating dependancies in Dockerfile #42 for updates to dependencies (no more pk bug) and moving away from the old artifact.
• ASAN + LSAN + optimisation level: I don't have a strong opinion on that, so whatever you (all) believe makes more sense, I'd be happy with - feel free to open a PR, before we officially release the CW (20th Feb = last lab)
• CI: I've added Action access to this year's (43) repos, thanks @Jpnock
• "Code quality, readability, and consistency in test.py": I don't see that point at all. I'd strongly argue the opposite, test.py went through multiple rounds of code review, it has documentation in most functions, typing support, clean abstraction and proper code reuse. That's on top of significantly more features than the older bash script. If there are any known bugs with the libraries we use in test.py, I'd assume there are also readily available workarounds, given we're using Python. The ~500 LoC of test.py isn't a great argument either imo, given that TA_test.sh in marking repo is close to 400 LoC. As for return code not being checked (thanks again @Jpnock), I've addressed that in Added return code check for cmake (configure + generate) #43.

My suggestion is we improve test.py if you feel like it isn't readable/trustworthy in the current state. Then, we actually create a new TA_test.py in marking which calls functions from test.py along with an anti-cheat and automatic code review we implemented last year, ported from TA_test.sh. Expanding on the automatic code review would also be a cool project, making our lives much easier around the marking period. My chain of thoughts are:

1. We plan to have langproc-cw as submodule in marking repo.
2. langproc-cw uses test.py which has been proven to work well for hundreds of students for a few years now.
3. Porting anti-cheat and automatic code review from bash to Python is simple, the difficult part of creating the logic is already done.
4. Expanding and modularising complex code in Python is simpler than bash, I also believe long files in Python are easier to maintain than bash, as most TAs would have more experience with Python projects. Compare with 14 LoC toolchain_test.sh which I think is a great example of where bash > Python.
5. By having a much closer connection between students' test script and TA's test script, we hopefully avoid any marking inconsistencies.

In rough pseudo-code, we end up with test.py:

<define clean(), build(), run_tests(), coverage()>
def main():
  clean()
  build()
  run_tests()
  coverage()

TA_test.py:

<import clean(), build(), run_tests()>
<define auto_code_review(), anti-cheat()>
def main():
  clean()
  build()
  auto_code_review()
  anti-cheat()
  run_tests()

Then the overall logic for early marking all repos / getting grade summary / Github Actions is simply to loop over repos while calling a subset of functions used in main() in TA_test.py - avoids the situation from 2 years ago with a lengthy separate mark_compiler.sh.

[johnwickerson]

Hi all, thanks for all the discussion!

Regarding the "multiple marking repos" issue: my rationale for making langproc-2023-marking, langproc-2024-marking, etc. was to do with access permission. I didn't want somebody who did the course in 2023 and then become a UTA in 2024 to be able to view all their marks (and those of all their peers), so I made a new repo for each year. The ta2023 team only has access to langproc-2023-marking and the ta2024 team only has access to langproc-2024-marking, and so on. But I recognise that there is an unnecessary about of duplication between these repos. Currently, each langproc-*-marking repo contains:

1. student marks .csv
2. testing scripts
3. directory of hidden tests

Really, it's only (1) that I actually want to keep hidden. So how about:

• I make a new langproc-marking repo, which contains testing scripts and a directory of hidden tests, and
• I make a langproc-YYYY-marking repo for each year, just containing student marks for that year.

And that langproc-YYYY-marking repo would contain langproc-marking as a submodule, which means it essentially symlinks to a particular commit of the langproc-marking repo (as was used when marking that particular year's compilers).

[johnwickerson]

One other point regarding coursework release: I've made all the student repos last week, and encouraged the students to get started on their coursework whenever they feel ready to do so. Several already have, and I know at least one group has finished (!). But I don't think it's a disaster if, over the next week or so, we adjust things like Ubuntu versions, or optimisation levels in testing scripts -- if it's a potentially breaking change, I can just tell the students to take note and make sure they use the most recent guidance -- they would still have several weeks to fix any minor issues that could result.

[Fiwo735]

Following up on langproc-marking (infrastructure) vs langproc-YYYY-marking (yearly) split, I've initialised the 2025 repo with:

• .gitmodules: adds langproc-marking as submodule, remember to run git submodule update --init --recursive after cloning.
• README.md: our original, slightly modified, README that explains the marking process. Compare with README.md in infrastructure repo, which has been shortened to avoid confusion.
• ta_allocation.csv: CSV with allocations, as before.
• create_TA_dirs.sh: simple bash script, to be run at the very start of the marking process by @johnwickerson after filling out ta_allocation.csv. This script creates a subdirectory for each TA, populated with markdown files for each group, based on template.md [WIP] automatically copied from the infrastructure repo.

These files are what needs to be copied over each year to a new langproc-YYYY-marking repo. While create_TA_dirs.sh could technically be kept in the infrastructure repo, I doubt it'd need any updates, so for simplicity's sake (easier paths) I include it in the yearly repo. Let me know if you think that's a poor design - happy to move to infrastructure repo as well.