Skip to content

test(engine): add ordered list regression coverage - #169

Merged
mttrbrts merged 2 commits into
accordproject:mainfrom
coldworld22:test/ordered-list-regression
Aug 5, 2026
Merged

test(engine): add ordered list regression coverage#169
mttrbrts merged 2 commits into
accordproject:mainfrom
coldworld22:test/ordered-list-regression

Conversation

@coldworld22

Copy link
Copy Markdown
Contributor

Summary

Add regression coverage for ordered list rendering when the list body starts with text followed by variables.

Changes

Testing

  • npm test -- TemplateMarkInterpreter

Notes

This PR only adds test coverage.
No production code, fixtures, or snapshots were changed.

Signed-off-by: Ayman Osman <aymanosman720@gmail.com>
@coldworld22
coldworld22 requested a review from a team July 26, 2026 09:37
@devanshi00

Copy link
Copy Markdown
Contributor

Hey @coldworld22,
Thanks for adding the relevant test that allows two failure modes get equal test coverage across both list types, rather than leaving one combination (olist + leading text).
Consider parametrizing instead of duplicating. Tests 1/2 and (now) 3/4 are structurally identical pairs that differ only in ulist/olist and 'bullet'/'ordered'. A test.each([['ulist','bullet'], ['olist','ordered']])(...) would remove the duplication and make it structurally impossible for this kind of asymmetry. The next time someone adds a new failure-mode test, they'd add one parametrized case instead of remembering to write both variants.
It is just a suggestion for future maintainability.
We will still need a review from @mttrbrts before final merge.

@coldworld22

coldworld22 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @devanshi00 for the suggestion. I’ve updated the issue #145 regression tests to use test.each for the ulist/olist cases across both failure modes, so future additions only need one parametrized case.

@devanshi00

Copy link
Copy Markdown
Contributor

Seems perfect for me!
@mttrbrts can you check this out ?

@mttrbrts
mttrbrts enabled auto-merge (squash) August 4, 2026 12:49
@mttrbrts
mttrbrts merged commit 19396ad into accordproject:main Aug 5, 2026
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants