Aman-Mittal commented on PR #695:
URL:
https://github.com/apache/fineract-backoffice-ui/pull/695#issuecomment-5971982202
Two more things, both found by looking at this pull request's own CI output.
## The change sequence diagram has never been posted on any pull request
Not a Mermaid problem — the diagram is never delivered. The `diagram` job
works and the artifact is there (`pr-comment-diagram`, 874 bytes, on CI run
37140999866), and the Mermaid it renders parses cleanly against mermaid 11's
own parser. The publisher skipped it every time and reported success, so the
only place it has ever appeared is the job summary.
`pr-comments.yml` downloaded with `pattern: pr-comment-*`, and at the pinned
SHA `download-artifact` resolves its destination as
```
isSingleArtifactDownload || mergeMultiple || artifacts.length === 1
? path
: path/<artifact name>
```
CI renders only the diagram, so the pattern always matched **exactly one**
artifact and the file landed at `summaries/pr-diagram.md` with no directory at
all. That directory name is how `post-e2e-comments.js` binds a body to the
marker it is allowed to post under, so a missing directory is indistinguishable
from an artifact that was never uploaded. The E2E summaries were never
affected, because that run uploads two — which is why half the mechanism looked
healthy for six weeks.
The workflow's own comment had anticipated this case and got the behaviour
backwards:
> Each artifact keeps its own directory, so the script can tell one body
from another by name … A run that produced only some of them — CI renders the
diagram, E2E the summaries — simply has fewer directories, and the script skips
what is absent.
With one, there are no directories.
Each artifact is now downloaded **by name, into a directory named after
it**, so the layout no longer depends on how many a run happened to produce.
Two further defects in the same mechanism went with it:
- The concurrency group was `<this workflow's name>-<commit>` — and
`github.workflow` is this workflow's own name, identical whichever run
triggered it. CI's publisher and E2E's publisher therefore shared a group, and
with `cancel-in-progress` whichever finished second killed the first. Now keyed
on `workflow_run.workflow_id` as well; the numeric id rather than the name,
because a `pull_request` run builds its workflow from the pull request's own
head.
- Posting nothing logged three "skipping" lines under a green check. It now
reports what `summaries/` actually held, and names a root-level `.md` as the
collapsed-layout symptom.
`post-e2e-comments.js` had no tests at all. It has seven, including one that
pins the agreement between the allow-list, the `upload-artifact` names and the
download directories; restoring the pattern download fails it with "an
allow-listed artifact is not downloaded". That check matters more than usual
here, because **this fix cannot be exercised by the pull request that makes
it** — a `workflow_run` workflow always runs the copy on the default branch, so
the first real run is the first after merge. If the diagram comment appears on
the pull request after this one, it worked.
## The real-Fineract failure on this branch was mine, and the break was
correct
`rbac-dead-end-controls.spec.ts` › *says the list was refused instead of
showing an empty table* seeded a user holding `READ_OFFICETRANSACTION` and not
`READ_OFFICE`, then asserted the screen loaded and the list request failed —
the bare column headers described in §4 above.
Gating the route on `READ_OFFICE` closes that door, so the test was
asserting a dead end this branch removes. It now asserts the refusal instead,
including that the query string names the code the user is **missing** rather
than the one they hold.
The component still carries the refused-load state, correctly, for every
other way the request can fail — and no user can reach it through permissions
any more. So it moves to a unit test, which also covers a load that merely
failed and a payload that is not the array the endpoint claims. The request is
driven through `HttpTestingController` rather than by mocking the generated
`DefaultService`, which would have put an ADR 0006 boundary violation in a test
file.
8/8 in that spec against a running Fineract, first attempt, no retries.
Worth noting four of its cases needed two retries on CI and none locally, which
is contention on the shared instance rather than the specs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
