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.

Sign up for GitHub

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

Jump to bottom

fix(common): flaky KeyConverter unit tests #299

Closed
petermetz opened this issue Oct 1, 2020 · 2 comments
Closed

fix(common): flaky KeyConverter unit tests #299

petermetz opened this issue Oct 1, 2020 · 2 comments
Labels
bug Something isn't working Hacktoberfest Hacktoberfest participants are welcome to take a stab at issues marked with this label. Security Related to existing or potential security vulnerabilities
Milestone
v0.2.0

Comments

@petermetz
Copy link
Member

Describe the bug

Failed once on the CI server but works just fine locally.
https://travis-ci.org/github/hyperledger/cactus/jobs/732084285

To Reproduce

  1. Checkout the commit 5938f28
  2. npm i && npm run run-ci
  3. Observe that succeeds (or does it?)

Expected behavior

Test case outcome should always be consistent across all environments that the tests can be executed in regardless of OS/number of cores/hardware differences in general.

Logs/Stack traces

https://travis-ci.org/github/hyperledger/cactus/jobs/732084285#L5538

Operating system name, version, build:

The Travis CI environment both Node 12 and 14

Hyperledger Cactus release version or commit (git rev-parse --short HEAD):

5938f28

Hyperledger Cactus Plugins/Connectors Used

  • Besu

Additional context

I'm suspicious that it may be the key generation in the beginning of the file packages/cactus-common/src/test/typescript/unit/key-converter.test.ts that goes on for too long without a timeout and then crashes the process, but no evidence of this just yet.
@petermetz petermetz added the bug Something isn't working label Oct 1, 2020
@petermetz petermetz added this to the v2.0.0 milestone Oct 1, 2020
@petermetz petermetz modified the milestones: v2.0.0, v0.5.0 Oct 7, 2020
@petermetz petermetz added Security Related to existing or potential security vulnerabilities and removed Nice-to-Have labels Oct 7, 2020
@petermetz
Copy link
Member Author

Modified milestone from 2.0.0 to 0.5.0 as per proposal from @sfuji822 (so that we include this in the MVP)

@petermetz petermetz added the Hacktoberfest Hacktoberfest participants are welcome to take a stab at issues marked with this label. label Oct 13, 2020
petermetz added a commit to petermetz/cacti that referenced this issue Oct 14, 2020
@petermetz
test(common): experiment to fix flaky test case
7f6ff4b 
Might not work out, but worth a try to refactor it this way:
Moved all the test setup logic to the
test cases
instead of having it dangle in the global context.

Fixes hyperledger#299 (potentially)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Oct 17, 2020
@petermetz
test(common): experiment to fix flaky test case
1db9afb 
Might not work out, but worth a try to refactor it this way:
Moved all the test setup logic to the
test cases
instead of having it dangle in the global context.

Fixes hyperledger#299 (potentially)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Oct 19, 2020
@petermetz
test(common): experiment to fix flaky test case
3a1f9cd 
Might not work out, but worth a try to refactor it this way:
Moved all the test setup logic to the
test cases
instead of having it dangle in the global context.

Fixes hyperledger#299 (potentially)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Oct 19, 2020
@petermetz
test(common): experiment to fix flaky test case
1bc445c 
Might not work out, but worth a try to refactor it this way:
Moved all the test setup logic to the
test cases
instead of having it dangle in the global context.

Fixes hyperledger#299 (potentially)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Oct 19, 2020
@petermetz
test(common): experiment to fix flaky test case
c62a1fc 
Might not work out, but worth a try to refactor it this way:
Moved all the test setup logic to the
test cases
instead of having it dangle in the global context.

Fixes hyperledger#299 (potentially)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Oct 20, 2020
@petermetz
test(common): experiment to fix flaky test case
4f3e076 
Might not work out, but worth a try to refactor it this way:
Moved all the test setup logic to the
test cases
instead of having it dangle in the global context.

Fixes hyperledger#299 (potentially)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Oct 22, 2020
@petermetz
test(common): experiment to fix flaky test case
4204fe6 
Might not work out, but worth a try to refactor it this way:
Moved all the test setup logic to the
test cases
instead of having it dangle in the global context.

Fixes hyperledger#299 (potentially)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Oct 26, 2020
@petermetz
test(common): experiment to fix flaky test case
08b57c8 
Might not work out, but worth a try to refactor it this way:
Moved all the test setup logic to the
test cases
instead of having it dangle in the global context.

Fixes hyperledger#299 (potentially)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Oct 27, 2020
@petermetz
test(common): experiment to fix flaky test case
c445adc 
Might not work out, but worth a try to refactor it this way:
Moved all the test setup logic to the
test cases
instead of having it dangle in the global context.

Fixes hyperledger#299 (potentially)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Oct 27, 2020
@petermetz
test(common): experiment to fix flaky test case
cad622e 
Might not work out, but worth a try to refactor it this way:
Moved all the test setup logic to the
test cases
instead of having it dangle in the global context.

Fixes hyperledger#299 (potentially)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
Copy link
Member Author

Finally had the time to investigate this properly and it is an issue that traces back to the tap test executor unfortunately.
We already had plans to replace tap with tape for other reasons (adding browser testing support) but now this has become more important than ever.
While seeing if we could use tape as the test runner as well, I arrived at the conclusion that we cannot because it does not support obtaining coverage while also compiling the Typescript code on the fly.
Another executor that does support this and also satisfies our requirement of being able to spit out TAP formatted results is AVA so I will update the relevant GH issue #238 about test runner migration to target AVA instead of tape...

@petermetz petermetz mentioned this issue Nov 2, 2020
Open
petermetz added a commit to petermetz/cacti that referenced this issue Nov 2, 2020
@petermetz
test(common): experiment to fix flaky test case
d491964 
Might not work out, but worth a try to refactor it this way:
Moved all the test setup logic to the
test cases
instead of having it dangle in the global context.

Fixes hyperledger#299 (potentially)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Nov 2, 2020
@petermetz
fix(common): flaky KeyConverter unit tests
ed5eda5 
Fixes hyperledger#299

This does not provide a real fix for the
issue just marks the tests to be skipped
for now until we complete the migration
to a different test runner which will
implicitly provide the real fix as per
hyperledger#299 (comment) =>

Finally had the time to investigate this
properly and it is an issue that traces
back to the tap test executor unfortunately.
We already had plans to replace tap with
tape for other reasons (adding browser
testing support) but now this has
become more important than ever.

While seeing if we could use tape as the
test runner as well, I arrived at the
conclusion that we cannot because it
does not support obtaining coverage
while also compiling the Typescript code on the fly.
Another executor that does support this
and also satisfies our requirement of
being able to spit out TAP formatted
results is AVA so I will update the
relevant GH issue hyperledger#238 about test
runner migration to target AVA instead of tape...

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Nov 6, 2020
@petermetz
test(common): experiment to fix flaky test case
a3bb9f6 
Might not work out, but worth a try to refactor it this way:
Moved all the test setup logic to the
test cases
instead of having it dangle in the global context.

Fixes hyperledger#299 (potentially)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Nov 6, 2020
@petermetz
fix(common): flaky KeyConverter unit tests
780e3af 
Fixes hyperledger#299

This does not provide a real fix for the
issue just marks the tests to be skipped
for now until we complete the migration
to a different test runner which will
implicitly provide the real fix as per
hyperledger#299 (comment) =>

Finally had the time to investigate this
properly and it is an issue that traces
back to the tap test executor unfortunately.
We already had plans to replace tap with
tape for other reasons (adding browser
testing support) but now this has
become more important than ever.

While seeing if we could use tape as the
test runner as well, I arrived at the
conclusion that we cannot because it
does not support obtaining coverage
while also compiling the Typescript code on the fly.
Another executor that does support this
and also satisfies our requirement of
being able to spit out TAP formatted
results is AVA so I will update the
relevant GH issue hyperledger#238 about test
runner migration to target AVA instead of tape...

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Nov 12, 2020
@petermetz
test(common): experiment to fix flaky test case
1c12daf 
Might not work out, but worth a try to refactor it this way:
Moved all the test setup logic to the
test cases
instead of having it dangle in the global context.

Fixes hyperledger#299 (potentially)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit to petermetz/cacti that referenced this issue Nov 12, 2020
@petermetz
fix(common): flaky KeyConverter unit tests
51ef027 
Fixes hyperledger#299

This does not provide a real fix for the
issue just marks the tests to be skipped
for now until we complete the migration
to a different test runner which will
implicitly provide the real fix as per
hyperledger#299 (comment) =>

Finally had the time to investigate this
properly and it is an issue that traces
back to the tap test executor unfortunately.
We already had plans to replace tap with
tape for other reasons (adding browser
testing support) but now this has
become more important than ever.

While seeing if we could use tape as the
test runner as well, I arrived at the
conclusion that we cannot because it
does not support obtaining coverage
while also compiling the Typescript code on the fly.
Another executor that does support this
and also satisfies our requirement of
being able to spit out TAP formatted
results is AVA so I will update the
relevant GH issue hyperledger#238 about test
runner migration to target AVA instead of tape...

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz added a commit that referenced this issue Dec 1, 2020
@petermetz
fix(common): flaky KeyConverter unit tests
43ec924 
Fixes #299

This does not provide a real fix for the
issue just marks the tests to be skipped
for now until we complete the migration
to a different test runner which will
implicitly provide the real fix as per
#299 (comment) =>

Finally had the time to investigate this
properly and it is an issue that traces
back to the tap test executor unfortunately.
We already had plans to replace tap with
tape for other reasons (adding browser
testing support) but now this has
become more important than ever.

While seeing if we could use tape as the
test runner as well, I arrived at the
conclusion that we cannot because it
does not support obtaining coverage
while also compiling the Typescript code on the fly.
Another executor that does support this
and also satisfies our requirement of
being able to spit out TAP formatted
results is AVA so I will update the
relevant GH issue #238 about test
runner migration to target AVA instead of tape...

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz petermetz removed this from the v0.5.0 milestone Dec 2, 2020
@petermetz petermetz added this to the v0.2.0 milestone Dec 2, 2020
ryjones pushed a commit that referenced this issue Feb 1, 2023
@sandeepnRES
Merge pull request #299 from michiboo/ERCAny
835f0df 
add interoperability between ERC20, ERC721 and ERC1155 in Besu network
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working Hacktoberfest Hacktoberfest participants are welcome to take a stab at issues marked with this label. Security Related to existing or potential security vulnerabilities
Projects
None yet
Milestone
v0.2.0
Development

No branches or pull requests
1 participant
@petermetz