Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Markdown Message Ordering got shuffled? #1014

Closed
fbartho opened this issue Mar 6, 2020 · 10 comments · Fixed by #1336
Closed

Bug: Markdown Message Ordering got shuffled? #1014

fbartho opened this issue Mar 6, 2020 · 10 comments · Fixed by #1336

Comments

@fbartho
Copy link
Member

fbartho commented Mar 6, 2020

Example code:

	markdown(assembleTable(recentBuildHeaders, recentBuildLinks));
	markdown(
		makeDisclosableBlock(
			`All commits with builds for ${currentBranchName}`,
			assembleTable(allBuildHeaders, allBuildLinks),
		),
	);
	markdown(
		"#### Latest Build Status Pages\n" +
			assembleTable(buildBadgeTableHeaders, [buildBadgeTableValues]),
	);

This code hasn't been changed since 2019-03-29

Expected Rendering (from last week):
screenshot of danger comment in expected layout

Yesterday's Rendering:
screenshot of danger comment with some of the markdown messages out of order

If you compare the code, to the renderings, the "Latest Build Status Pages" table is expected to be last.

It appears we're using danger@8.0.0 -- will try updating, to see what happens.

@fbartho
Copy link
Member Author

fbartho commented Mar 6, 2020

Updating to danger@9.2.10 did not resolve this issue. It appears that markdown entries are being flipped in order when rendering to a comment.

@orta
Copy link
Member

orta commented Mar 7, 2020

It should be deterministic - so maybe it's just deterministically reversed.

@fbartho
Copy link
Member Author

fbartho commented Mar 7, 2020

The problem I see is that based on the code, it’s not deterministic - otherwise I could fix this! The top entry of 3 (in code) is shuffled to the bottom (in screenshot 2)

@orta
Copy link
Member

orta commented Mar 8, 2020

Ah, fair - I guess it's not unreasonable to record the order when a violation is created to ensure it's consistent on both sides.

I'm pretty sure there are some tests which might fail if it's indeterminate, but it's been a while since I've looked at this codebase

@fbartho
Copy link
Member Author

fbartho commented Mar 8, 2020

I could rewrite my code, to combine these markdown messages into one call, so I have a workaround for myself. Not sure if this is valuable to try to track down?

@orta
Copy link
Member

orta commented Mar 8, 2020

Looks like it's been around while #845 - so if no-one else has also thought it important enough to work on, that's fine IMO

@falkenhawk
Copy link
Contributor

Is there a possibility to fix this issue? 🙏

@falkenhawk
Copy link
Contributor

falkenhawk commented Jul 9, 2020

my code looks like this:

function doStuff(n) {
  return Promise.all([...]).then(res => {
     markdown(`message ${n}`);
  });
}

schedule(async () => {
  await doStuff(1);
  await doStuff(2);
  await doStuff(3);
});

and testing locally with danger pr always yields: (maybe i was just lucky to get the correct order in all test runs 🤔)

## Markdowns
message 1
message 2
message 3

while running this in github workflow, the resulting order of the markdown strings are non-deterministic. Sometimes it's in correct order, sometimes it's shuffled.

ofc my real doStuff function is much more complex and actually comes from an external plugin, so not much there for me to hijack the messages and concatenate them manually before calling markdown() once :(

EDIT: it seems to affect not only markdown() but also message()/warn() in the same way

@orta
Copy link
Member

orta commented Jul 9, 2020

You're welcome to fix it 👍

Maybe add a creation date to the Violations and then sort it at the end

@falkenhawk
Copy link
Contributor

You're welcome to fix it 👍

#1336 @orta

@orta orta closed this as completed in #1336 Nov 3, 2022
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 a pull request may close this issue.

3 participants