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

Change word length due to error on mechanical-markdown #1279

Merged
merged 3 commits into from
Mar 25, 2025

Conversation

mcruzdev
Copy link
Contributor

@mcruzdev mcruzdev commented Mar 24, 2025

Description

This pull request adjust the mechanical-markdown scenario due to an issue that lives on mechanical-markdown lib.

Closes #1266

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@mcruzdev mcruzdev requested review from a team as code owners March 24, 2025 16:39
@mcruzdev mcruzdev force-pushed the issue-1266 branch 2 times, most recently from 0c17158 to 3e54d98 Compare March 24, 2025 18:08
@mcruzdev
Copy link
Contributor Author

@cicoyle @salaboy could you take a look?

@salaboy
Copy link
Contributor

salaboy commented Mar 25, 2025

@mcruzdev looking at the code changes.. I am confused on how this fixes the flaky test.. I remember seeing a comment somewhere where it was explained.. but I cannot find it now.

@salaboy
Copy link
Contributor

salaboy commented Mar 25, 2025

Ok found it.. #1266 (comment)
but if the issue is with the order, then there is a bug with match_order: none on mechanical markdown right?

I still don't understand how changing the word count fixes the issue. @mcruzdev maybe you can share the sequence of things that happened and what are the expected values to make it easier to review.

@salaboy
Copy link
Contributor

salaboy commented Mar 25, 2025

Ok for context, we are looking at the execution of this test: https://github.com/dapr/java-sdk/blob/3e54d98cf056162dc3bc237f337a359bffa8be18/examples/src/main/java/io/dapr/examples/workflows/README.md#fan-outfan-in-pattern

This test executes five activities in parallel. Each activity counts the words (tokens), and then the workflow sums up all the results.

The issue seems to be here:

where only 4 of 5 activities are expected.. hence depending on the order one might be missed, or removed as @mcruzdev mentioned. There are no five checks for all the activity results, I think if we add another check (and make sure that the checks are not in order) we should get a more consistent behavior.

Ok.. so changing the word count makes sense because of this: dapr/mechanical-markdown#37

@salaboy
Copy link
Contributor

salaboy commented Mar 25, 2025

Ok.. now I think I understand the full context.. I would recommend to change the output
Right now - 'Activity returned: 2' and - 'Activity returned: 21' are clashing. We can quickly fix that by changing the output format.

For example.. we can just add a period at the end and match with the period, for example:

'Activity returned: 2.'
'Activity returned: 21.'

Those two will not clash when compared by mechanical markdown right? @mcruzdev
This change will keep it simple and will not change the input text for the test.

Does this make sense?

@mcruzdev
Copy link
Contributor Author

Yes, it makes sense 😅

@mcruzdev mcruzdev force-pushed the issue-1266 branch 3 times, most recently from a606832 to c5ba0d6 Compare March 25, 2025 09:38
Copy link
Contributor

@cicoyle cicoyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for looking into this!

@cicoyle cicoyle merged commit 16f9da0 into dapr:master Mar 25, 2025
7 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.

Fix Flaky Workflow Test Failure
4 participants