Feedback - Checkpoint 1 (Lab 1 + Lab 2)¶

This document collects the most common issues and tips from grading the Checkpoint 1 submissions. Many groups did a lot of things very well: clear rule-based mission logic, solid unit tests, working pipelines, deliberate merge-protection tests. What follows are the points where we saw the largest gaps between groups, and how to avoid them in the next labs. Nothing here is aimed at a single group: almost every point was seen in several teams.

Tag, pipeline and images: the hand-in checklist¶

The Checkpoint1 tag is what we use to find your final code, and its pipeline is what produces your Checkpoint1 and latest images. Several groups had a tag that did not do its job:

The tag was created before the final merge and therefore pointed to an older commit. Any MR merged afterwards (for example the Jib publish MR or the last feature) was not part of the tagged code and not part of the tagged image.
The publish job was not on main yet when the tag was pushed (the MR was still open). The tag pipeline then only ran verify, and no Checkpoint1 or latest image was ever created.
The tag was lightweight instead of annotated, or the message was used as the tag name (for example Final-commit-for-Checkpoint-1). The name must be exactly Checkpoint1.

A short checklist before you tag:

All MRs for the checkpoint are merged and the pipeline on main is green.
git checkout main && git pull, then tag that commit with an annotated tag:

git tag -a Checkpoint1 -m "Final commit for Checkpoint 1."
git push origin Checkpoint1

In the UI (Code > Tags > New Tag) fill in the Message field, otherwise GitLab creates a lightweight tag.

Wait for the tag pipeline, open the container registry, and check that the Checkpoint1 and latest tags exist.
Link that exact pipeline in your report.

If you mistagged: delete the tag and create it again on the right commit. That is perfectly fine, as long as it happens before the deadline.

A related detail: use the image name that GitLab gives you ($CI_REGISTRY_IMAGE) in the Jib command. One group published to $CI_REGISTRY_IMAGE/logic-service, which creates a nested repository .../logic-service/logic-service. It works, but nobody expects the images there.

Use a work item for your report¶

The report must be a work item titled Checkpoint 1 Report with the labels Report and Checkpoint 1 (exactly these names, not Report::1 or Checkpoint::1) and the template as its description. One group committed the filled-in template as a file in the repository instead.
Delete the template prompts and the instruction comment once a section is answered. A report that still contains _Which convention do we use? Give an example._ looks unfinished.
Link the exact evidence, not a list page:
the milestone itself,
the pipeline of the tag (/-/pipelines/12345, not /-/pipelines or the tag page),
at least one merged MR per member and who approved it,
for the red-green MR: the MR, the work item, and the stub, failing-test and implementation commits.
Re-check every claim against GitLab. Reports said "every MR closes its work item", "we rebase to keep a linear history" or "every MR was approved", while the MR list and the commit history showed otherwise. A report that does not match the evidence is worse than a short report.
The label table should describe the labels you actually use, not label groups ("priority", "type").

Work items are your specification¶

The lab asks that every rule gets a work item with an owner, a priority and the specification of the rule before any code is written. Work items with an empty description, or with a single line ("Resource ophalen"), were the single most common planning issue. A short template already goes a long way:

## Information available

The squad member sees `locationsInView` (3x3 around it) and the landing craft location
from the mission control state. It does not see deposits outside its view.

## Action and preconditions

`DropOffResourceAction` when the member stands on the landing craft and carries > 0.

## Precedence

Drop-off goes before extract; extract goes before moving to a deposit.

## Edge cases

- crate exactly full
- landing craft moved after a wipeout (always read `landingCraftLocation()`)
- empty crate on the landing craft: no drop-off

Also: put all work items and MRs of the checkpoint in the milestone, give the milestone the right name and due date, close items when they are done (an MR that says Closes #12 does it for you), and create a work item for every deliverable (movement, logging, fixtures, integration tests, ...). Some groups only had three or four items for a whole checkpoint.

Merge requests: scope, description, link¶

An MR should be a small, reviewable unit that you can trace back to a work item. What we saw most:

No link to a work item. Put Closes #n (or Related to #n) in the description, not only in the title. Then the work item is closed on merge and shows the MR.
Empty descriptions and generic titles (Main, Testen, Feature/ci cd docker). Say what changed, why, and how a reviewer can verify it:

## What

Keep scan discoveries in a per-mission list that survives `PUT /missions/{id}`.

## How to verify

`MissionApiIT.scannedDepositSurvivesTurnUpdate`

Closes #9

Labels and milestone missing on half of the MRs.
Merging main into a feature branch through an extra MR, or opening and closing several MRs for the same branch. Use git merge origin/main locally (or the Update branch button) and keep one MR per branch.
Everything on the last day. Several groups merged most of their work in the last 24 hours, with MRs only open for minutes. A review of a ten-minute-old MR is not a review. Merge small MRs continuously; you will also find your problems earlier.

Make the pipeline actually block merging¶

The verification job is only useful if a red pipeline cannot be merged. Check the settings, don't assume:

Settings > Repository > Branch rules > main: allowed to push: No one.
Settings > Merge requests: Pipelines must succeed enabled (one group did not enable it, so a failing pipeline would not have blocked anything), plus an approval rule that requires at least 1 approval (some groups had a rule with 0, or merged their first MRs before the rule existed).
Test it with a deliberately failing branch, with a pipeline that really runs. In one case the "failing branch" was opened before .gitlab-ci.yml existed on main, so no pipeline ran and nothing was proven. Link the failed pipeline in your report.
If you use workflow:rules, take the block from the lab so that a branch with an open MR does not get two pipelines (a branch pipeline and an MR pipeline for the same commit) or use the GitLab project Settings>CI/CD>General pipelines>Skip branch pipelines for merge requests. The lab's workflow block is:

workflow:
  rules:
    - if: $CI_PIPELINE_SOURCE == "merge_request_event"
    - if: $CI_COMMIT_BRANCH && $CI_OPEN_MERGE_REQUESTS
      when: never
    - if: $CI_COMMIT_BRANCH
    - if: $CI_COMMIT_TAG

Do not hide a missing executable bit with chmod +x ./mvnw in every job. Fix it once and commit it:

git update-index --chmod=+x mvnw
git commit -m "chore: make the Maven wrapper executable"

Tip: cache your Maven dependencies¶

Almost nobody added dependency caching (it was optional), but it makes every pipeline noticeably faster. GitLab can only cache paths inside the project directory, so move the local Maven repository there and cache it per branch:

variables:
  MAVEN_OPTS: "-Dmaven.repo.local=.m2/repository"

cache:
  key: ${CI_COMMIT_REF_SLUG}
  paths:
    - .m2/repository

A job that does not need dependencies (a job that only runs the game, for instance) can switch the cache off with cache: []. Use cache for dependencies and artifacts for build output that a later job in the same pipeline needs.

Test-first: show the red state in CI¶

Many commit histories looked like TDD, with test: ... (red) followed by feat: ... (green). But in most of them the red commit was pushed together with the green commit, so GitLab only ever ran a pipeline on the green one, and nobody can see that the test failed first. Two things go wrong here:

Push the red commit on its own, let the pipeline fail, then push the implementation. The failed pipeline is your evidence.
The red must be an assertion failure, not a compilation error. A test for a class that does not exist yet fails with cannot find symbol, which only proves that the class is missing. First commit a minimal stub (for example returning NoAction), then the test:

# not a useful red:
[ERROR] COMPILATION ERROR: cannot find symbol  class ScanStateService

# a useful red:
[ERROR] MissionApiIT.depositIsKeptAfterTurnUpdate:121
        Status expected:<204 NO_CONTENT> but was:<404 NOT_FOUND>

Also make sure the scan-state rule gets its own red-green MR (several groups wrote it in one commit), and set up the verify pipeline first, before you develop the rules. A pipeline introduced halfway makes the earlier cycles unverifiable.

Integration tests: test the application over HTTP¶

An integration test starts the real application and talks to it through its endpoints. Things we saw:

A test class named ...IT that only contains contextLoads(), or a "controller test" that builds the controller by hand with mocks.
Missing: the 404 for unknown mission ids on every endpoint that takes an id (PUT, request-action, scan-completed and DELETE).
Missing: the lifecycle (start, request-action, scan, update, delete), state is gone after the mission ended, and that a scanned deposit survives a turn update and changes a later action.

Use RANDOM_PORT and RestTestClient, as the lab shows:

@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
@AutoConfigureRestTestClient
class MissionApiIT {

    @Autowired
    RestTestClient client;

    @Test
    void unknownMissionReturns404() {
        client.delete().uri("/missions/unknown").exchange().expectStatus().isNotFound();
        client.post().uri("/missions/unknown/scan-completed")
                .body(new DepositFoundEvent(1, new Location(3, 4), ResourceType.METAL))
                .exchange().expectStatus().isNotFound();
        // ... and request-action and PUT
    }
}

Unit tests, on the other hand, belong in *Test classes (run by Surefire) and should be small, one behaviour per test, with positive, negative and boundary cases per rule: a crate that is exactly full, a deposit with quantity == 0, a landing craft with an empty crate.

Mission logic: the same pitfalls came back in several groups¶

Only target deposits that still contain resources. Checking instanceof ResourceDeposit is not enough:

entity instanceof ResourceDeposit deposit && deposit.getQuantity() > 0

Forget scanned deposits that turn out to be gone. If a member walks to a scanned deposit that has been emptied and the scan stays in the list, it keeps walking to (and standing on) that tile forever. When a scanned location is in view and holds no deposit with resources, remove it from the mission state. If a member stands on it, remove it as well.
Return 404 for every unknown mission id, including PUT, DELETE and request-action, so the backend can re-send POST /missions. Returning 200 with NoAction or 204 on delete breaks that recovery.
The map is a torus. Compute every coordinate with Math.floorMod and take the shorter way around:

static int shortestDelta(int from, int to, int size) {
    int delta = Math.floorMod(to - from, size);
    return delta > size / 2 ? delta - size : delta;
}

Also test on a rectangular map; a width/height mix-up stays invisible on a 10x10 test map.

Clean up per mission on DELETE and when a mission is started again, and keep per-mission state in thread-safe structures (ConcurrentHashMap, CopyOnWriteArrayList): requests arrive concurrently.
Keep the logic out of the controller and avoid hard-coded numbers such as capacity - 10. Derive the decision from the crate capacity and the stored amount, and put the rules in a service behind an interface (the groups with one class per rule were the easiest to test and to read).

Logging: configure it and use the levels¶

Setting the logging level is part of the lab. Without logging.level...: DEBUG in application.yaml, none of your decision logging is visible. Replace the placeholder colonyN in the example with your own package; a config for a package that does not exist changes nothing.
Use the levels deliberately: INFO for the mission lifecycle and new turns, WARN for unknown mission ids (the 404s), DEBUG for the reasoning behind one squad member's decision.
Use {} placeholders, not string concatenation. Concatenation (and calls such as LocalTime.now()) is evaluated even when DEBUG is off. Guard genuinely expensive arguments with log.isDebugEnabled():

// not like this
log.debug(LocalTime.now() + " [RETURN] " + member.getId() + " carries " + carried);

// like this
log.debug("{} at {}: crate {}/{} -> returning to landing craft", id, location, carried, capacity);

Hygiene that makes the difference between "works" and "professional"¶

Remove commented-out code, TODOs and outdated comments ("returns 501 Not Implemented" on an endpoint that now returns 204) before you hand in. Remove old versions of jobs from .gitlab-ci.yml; git remembers them.
Test fixtures belong in src/test/java, not in src/main, where they ship with your application.
Follow the package layout of the lab (controllers/ and logic/ inside your logic_service package). A different base package name is fine as long as the logging configuration matches it.
Use the model-lib version announced for the lab, and keep your application.yaml and pom.xml tidy.
Review in practice: use the Approve button (a comment is not an approval), and make sure each member has reviewed at least one MR.