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

Fix a test case with a undefined result #42

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

TonyPythoneer
Copy link
Contributor

@TonyPythoneer TonyPythoneer commented Jul 23, 2024

Root cause

If result is undefined, it should add a ? after result to avoid the potential error.

  ● Durable Functions › should be able to run an empty orchestration

    expect(received).toBeUndefined()

    Received: {"_errorType": "TypeError", "_message": "Cannot read properties of undefined (reading 'Symbol(Symbol.asyncIterator)')", "_stackTrace": "TypeError: Cannot read properties of undefined (reading 'Symbol(Symbol.asyncIterator)')

      129 |             const resultType = result?.toString();
      130 |
    > 131 |             const isAsyncGenerator = typeof result[Symbol.asyncIterator] === 'function';
          |                                                   ^
      132 |             if (isAsyncGenerator) {
      133 |               // Start the orchestrator's generator function
      134 |               await ctx.run(result);

Ref: https://github.com/microsoft/durabletask-js/actions/runs/10032489350/job/27763486093


Else

I add Object.prototype.toString.call to detect object class.
I think this way is more correct when referring to the MDN document.

Ref: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/toString#using_tostring_to_detect_object_class

@TonyPythoneer TonyPythoneer changed the title [TODO] Fix/undefined case Fix a test case with a undefined result Jul 23, 2024
@TonyPythoneer
Copy link
Contributor Author

@kaibocai @cgillum @XavierGeerinck @microsoftopensource
Are you available to review my PR?

Thank you!

@kaibocai
Copy link
Member

Hi @TonyPythoneer , thanks for your contribution, would you mind helping check why the CI is failing? Thanks.

@TonyPythoneer
Copy link
Contributor Author

@microsoft-github-policy-service agree

@TonyPythoneer
Copy link
Contributor Author

TonyPythoneer commented Jul 25, 2024

Hi @kaibocai ,

The failed case comes from should be able to use the sub-orchestration for fan-out of test/e2e/orchestration.spec.ts.

It seems to run 10 sub-orchestrations, each with 3 activities, which is too many if it is just testing its functionality.
To make it easier, I set 2 sub-orchestrations and each sub-orchestration for 2 activities. It can avoid the timeout problem there.

Please have a look.

Thank you!


By the way, this is my test result from my local.

PASS  test/e2e/orchestration.spec.ts (37.117 s)
  Durable Functions
    ✓ should be able to run an empty orchestration (2783 ms)
    ✓ should be able to run an activity sequence (2314 ms)
    ✓ should be able to run fan-out/fan-in (2127 ms)
    ✓ should be able to use the sub-orchestration (2101 ms)
    ✓ should be able to use the sub-orchestration for fan-out (2149 ms)
    ✓ should allow waiting for multiple external events (2079 ms)
    ✓ should be able to run an single timer (5086 ms)
    ✓ should wait for external events with a timeout - true (2052 ms)
    ✓ should wait for external events with a timeout - false (5052 ms)
    ✓ should be able to terminate an orchestration (2062 ms)
    ✓ should allow to continue as new (2103 ms)
    ✓ should be able to run an single orchestration without activity (2041 ms)
    ✓ should be able to purge orchestration by id (2070 ms)
    ✓ should be able to purge orchestration by PurgeInstanceCriteria (2151 ms)

Test Suites: 1 passed, 1 total
Tests:       14 passed, 14 total
Snapshots:   0 total
Time:        37.166 s
Ran all test suites matching /test\/e2e/i.

@kaibocai kaibocai merged commit dcf25af into microsoft:main Jul 25, 2024
2 checks passed
@kaibocai
Copy link
Member

Merged, thanks for your contribution! @TonyPythoneer

@TonyPythoneer
Copy link
Contributor Author

Hi @kaibocai ,

The failed case comes from should be able to use the sub-orchestration for fan-out of test/e2e/orchestration.spec.ts.

It seems to run 10 sub-orchestrations, each with 3 activities, which is too many if it is just testing its functionality. To make it easier, I set 2 sub-orchestrations and each sub-orchestration for 2 activities. It can avoid the timeout problem there.

Please have a look.

Thank you!

By the way, this is my test result from my local.

PASS  test/e2e/orchestration.spec.ts (37.117 s)
  Durable Functions
    ✓ should be able to run an empty orchestration (2783 ms)
    ✓ should be able to run an activity sequence (2314 ms)
    ✓ should be able to run fan-out/fan-in (2127 ms)
    ✓ should be able to use the sub-orchestration (2101 ms)
    ✓ should be able to use the sub-orchestration for fan-out (2149 ms)
    ✓ should allow waiting for multiple external events (2079 ms)
    ✓ should be able to run an single timer (5086 ms)
    ✓ should wait for external events with a timeout - true (2052 ms)
    ✓ should wait for external events with a timeout - false (5052 ms)
    ✓ should be able to terminate an orchestration (2062 ms)
    ✓ should allow to continue as new (2103 ms)
    ✓ should be able to run an single orchestration without activity (2041 ms)
    ✓ should be able to purge orchestration by id (2070 ms)
    ✓ should be able to purge orchestration by PurgeInstanceCriteria (2151 ms)

Test Suites: 1 passed, 1 total
Tests:       14 passed, 14 total
Snapshots:   0 total
Time:        37.166 s
Ran all test suites matching /test\/e2e/i.

I just attached the build history I was resolving here when it suffered from the timeout issue in the test cases.

https://github.com/microsoft/durabletask-js/actions/runs/10051155988/job/27783699271

FAIL test/e2e/orchestration.spec.ts (68.576 s)
  Durable Functions
    ✓ should be able to run an empty orchestration (2342 ms)
    ✓ should be able to run an activity sequence (2309 ms)
    ✓ should be able to run fan-out/fan-in (2103 ms)
    ✓ should be able to use the sub-orchestration (2078 ms)
    ✕ should be able to use the sub-orchestration for fan-out (32033 ms)
    ✓ should allow waiting for multiple external events (2075 ms)
    ✓ should be able to run an single timer (5051 ms)
    ✓ should wait for external events with a timeout - true (2051 ms)
    ✓ should wait for external events with a timeout - false (5050 ms)
    ✓ should be able to terminate an orchestration (2555 ms)
    ✓ should allow to continue as new (2109 ms)
    ✓ should be able to run an single orchestration without activity (2040 ms)
    ✓ should be able to purge orchestration by id (2055 ms)
    ✓ should be able to purge orchestration by PurgeInstanceCriteria (2142 ms)

  ● Durable Functions › should be able to use the sub-orchestration for fan-out

    TimeoutError

      173 |       const res = (await Promise.race([
      174 |         prom(req),
    > 175 |         new Promise((_, reject) => setTimeout(() => reject(new TimeoutError()), timeout * 1000)),
          |                                                            ^
      176 |       ])) as pb.GetInstanceResponse;
      177 |
      178 |       const state = newOrchestrationState(req.getInstanceid(), res);

      at Timeout._onTimeout (src/client/client.ts:175:60)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 13 passed, 14 total
Snapshots:   0 total
Time:        68.621 s
Ran all test suites matching /test\/e2e/i.
Error: The operation was canceled.

I hope this can help other viewers read here and get the complete context.

@TonyPythoneer TonyPythoneer deleted the fix/undefined-case branch July 26, 2024 00:12
@TonyPythoneer
Copy link
Contributor Author

TonyPythoneer commented Jul 26, 2024

Hi @kaibocai

I have another question.
Could durabletask-js release a newer version with the recent fixes?

I'd like to let dapr js sdk use a new version of durabletask-js for Workflow.

So, what's your release plan?

Thank you!

@kaibocai
Copy link
Member

Hi @TonyPythoneer , I don't have an estimate for the release. But I will note this down and start looking into it. Will keep you updated.

@kaibocai
Copy link
Member

We are migrating our release pipeline due to security requirements. I am actively working on it. I will keep you updated. Thanks.

@TonyPythoneer
Copy link
Contributor Author

Hi @kaibocai

I have a question.

Could I expect these changes to be updated indurabletask-js and dapr nodejs sdk during the Dapr 1.14 release?

Thank you!

@kaibocai
Copy link
Member

@TonyPythoneer , sorry for the late reply, I missed your previous message. The new release is here https://www.npmjs.com/package/@microsoft/durabletask-js. thanks

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.

2 participants