Skip to content

Conversation

@mao-sz
Copy link
Contributor

@mao-sz mao-sz commented Dec 30, 2025

Because

The current way of testing markdownlint custom rules is functional but not the most ideal. Ideal would be automated tests with a runner and against assertions, but until now has been a low priority thing due to potential complexities in testing CLI stuff with third-party libraries.

This is one beefy PR but should be more approachable by reviewing the individual commits, and all tests hopefully passing. The commits are for the most part separate from each other, shouldn't be many commits that change something about a previous commit.

Please do checkout into the PR locally and run npm run test just in case I've made errors with the GH workflow.

This PR

  • Documents markdownlint contribution protocol and expected formats for custom rules and tests
  • Adds Node assertion tests for all markdownlint custom rules
    • Involves many test .md files being renamed/split
  • Excludes markdownlint/ from standard markdownlint GH workflow (intended for lessons/projects)
  • Adds GH workflow specifically for running tests for markdownlint custom rules
  • Updates markdownlint-cli2 minimum version to 0.20.0 due to needing the --format CLI option for tests (released in 0.19.0)

Issue

Closes #30590
Closes #30575

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project curriculum contributing guide
  • The title of this PR follows the location of change: brief description of change format, e.g. Intro to HTML and CSS lesson: Fix link text
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If any lesson files are included in this PR, they have been previewed with the Markdown preview tool to ensure it is formatted correctly
  • If any lesson files are included in this PR, they follow the Layout Style Guide

@mao-sz mao-sz added Type: Chore Involves changes with no user-facing value, to the build process/internal tooling, refactors, etc. Content: Markdownlint Involves anything related to the curriculum repo linter labels Dec 30, 2025
@github-actions github-actions bot removed the Type: Chore Involves changes with no user-facing value, to the build process/internal tooling, refactors, etc. label Dec 30, 2025
@mao-sz mao-sz added the Type: Chore Involves changes with no user-facing value, to the build process/internal tooling, refactors, etc. label Dec 30, 2025
@mao-sz
Copy link
Contributor Author

mao-sz commented Dec 30, 2025

I opted to have the custom rule test workflow just run all tests on any custom rule/test file change, rather than try to mess with changed file paths and selectively running tests. That seemed like a hell of a lot of work for little gain, and I'd have to deal with ensuring changes to markdownlint/test_utils/ run all tests still.

For some reason the test workflow uses the TAP test reporter despite it just running npm run test 🤷 The "default" reporter spec is subjectively a touch nicer to read, though arguably not too different.
image

Missing headings not fixable but incorrect heading levels are, so the
test md files should be split.
@mao-sz mao-sz force-pushed the markdownlint-testing branch from 8b9cc87 to 94fc986 Compare December 30, 2025 14:33
@ManonLef ManonLef requested review from a team and JustWaveThings and removed request for a team December 30, 2025 22:30
@jove0610
Copy link
Contributor

just some nit, maybe we can just directly pass the full file path in the lint.js and fix.js function like this:

module.exports = async (markdownFileFullPath) => {
  // don't catch - we want this to throw and halt the tests if the file does not exist
  await access(markdownFileFullPath);

  try {
    await exec(`npm run lint -- "${markdownFileFullPath}"`);
    return [];
  } catch (error) {
    return error.stderr.trim().split("\n");
  }
}

and then on the test file we use it like this:

// before
// const getLintErrors = require("../../test_utils/lint")(__dirname);
// const fixLintErrors = require("../../test_utils/fix")(__dirname);

// after
const getLintErrors = require("../../test_utils/lint");
const fixLintErrors = require("../../test_utils/fix");

since I noticed if errorPath is defined in the test then that's already the full file path, if not defined we can just do path.join()

@mao-sz
Copy link
Contributor Author

mao-sz commented Jan 12, 2026

@jove0610 __dirname is the absolute dir path from root (e.g. /home/mao_sz/repos/TOP/curriculum/path/to/file) which is different from the errorPath variable in tests which only has the relative path from the repo root to match the lint error output. The full absolute path is needed instead for fs stuff.

With the way you've suggested, you'd have to pass in an absolute path from root for the test .md file for every getLinterErrors call, meaning you'd have to manually do path.join(__dirname, './path/to/file') for every test that uses a different .md file.

The way I've chosen this code to be written means getLintErrors is already set up in each file with the correct __dirname, and can therefore be called with a path to the test .md file that's relative to the test file itself (which allows for super quick autocomplete when you type ./ as an arg, which makes writing new tests simpler and more intuitive).

Basically, I don't want all the .test.js files to share the same lint/fix function, I want them each to have their own version of lint/fix preloaded with the appropriate __dirname path. That makes using it within the .test.js file simpler, making tests easier to understand at a glance and easier to write if new ones are needed.

@mao-sz mao-sz requested review from a team and thatblindgeye and removed request for a team and JustWaveThings January 13, 2026 04:08
@ManonLef ManonLef self-requested a review January 31, 2026 21:24
@mao-sz
Copy link
Contributor Author

mao-sz commented Feb 3, 2026

Thought I should include #30575 in this PR since it'd probably make reviewing easier

@mao-sz mao-sz force-pushed the markdownlint-testing branch from 6881cba to bd7017a Compare February 3, 2026 23:13
@mao-sz mao-sz force-pushed the markdownlint-testing branch from bd7017a to 36770e5 Compare February 3, 2026 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Content: Markdownlint Involves anything related to the curriculum repo linter Type: Chore Involves changes with no user-facing value, to the build process/internal tooling, refactors, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Markdownlint: Convert tests to automated assertion tests Markdownlint: Document contribution protocol

2 participants