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

getAccountAddress() broken - could be dependency issue #182

Closed
sisyphusSmiling opened this issue Sep 9, 2022 · 10 comments · Fixed by #186
Closed

getAccountAddress() broken - could be dependency issue #182

sisyphusSmiling opened this issue Sep 9, 2022 · 10 comments · Fixed by #186
Assignees
Labels
bug Something isn't working

Comments

@sisyphusSmiling
Copy link
Contributor

Problem

Running getAccountAddress() results in error. Because this may be a dependency issue, I will also make an issue in flow-cadut

Steps to Reproduce

As an example, running the following in a test using flow-js-testing:

test("getAccountAddress breaks", async () => {
        const testAddress = await getAccountAddress("TestAddress");
    });

Gets us the following output:

Cannot read properties of undefined (reading 'replace')
TypeError: Cannot read properties of undefined (reading 'replace')
    at Object.replace [as replaceImportAddresses] (/Users/gsanchez/Workspace/flow-core-contracts/lib/js/test/node_modules/@onflow/flow-cadut/src/imports.js:103:15)
    at replaceImportAddresses (/Users/gsanchez/Workspace/flow-core-contracts/lib/js/test/node_modules/@onflow/flow-js-testing/src/interaction.js:95:14)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

Alternatively, this can be reproduced with an existing repo - flow-nft's JS tests are no longer passing. Try pulling and running those and you should get something like the output quoted above.

Acceptance Criteria

getAccountAddress() runs without error

Context

Currently blocked from writing tests for the Execution Node versioning contract

@sisyphusSmiling sisyphusSmiling added the bug Something isn't working label Sep 9, 2022
@adbario
Copy link
Contributor

adbario commented Sep 14, 2022

Having the same issue here as @sisyphusSmiling. Downgraded to 0.3.0-alpha.14 for now to get things rolling again while waiting for a fix.

@gregsantos
Copy link
Member

gregsantos commented Sep 14, 2022

@sisyphusSmiling Thanks for reporting.
I just ran all examples from flow-js-testing including 01-get-account-address.test.js and cannot replicate that error.

test("get account address", async () => {
  const testAddress = await getAccountAddress("TestAddress")

  // Expect Alice to be address of Alice's account
  expect(isAddress(testAddress)).toBe(true)
})

Passes without error 🧐

Also I'm able to successfully run all tests from flow-nft

@adbario
Copy link
Contributor

adbario commented Sep 14, 2022

@gregsantos This is what I get with 0.3.0-alpha.15 on the following test:

it("should get the test account address", async () => {
  await getAccountAddress("TestAddress");
});
      at _callee$ (node_modules/@onflow/util-logger/src/util-logger.js:44:7)
      at tryCatch (node_modules/@babel/runtime/helpers/regeneratorRuntime.js:86:17)
      at Generator._invoke (node_modules/@babel/runtime/helpers/regeneratorRuntime.js:66:24)
      at Generator.next (node_modules/@babel/runtime/helpers/regeneratorRuntime.js:117:21)
      at asyncGeneratorStep (node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:24)
      at _next (node_modules/@babel/runtime/helpers/asyncToGenerator.js:25:9)

 FAIL  tests/hello-world.test.ts
  HelloWorld
    ✕ should get the test account address (636 ms)

  ● HelloWorld › should get the test account address

    TypeError: Cannot read properties of undefined (reading 'replace')

      31 |
      32 |   it("should get the test account address", async () => {
    > 33 |     const testAddress = await getAccountAddress("TestAddress");
         |                         ^
      34 |   });
      35 | });
      36 |

      at Object.replace [as replaceImportAddresses] (node_modules/@onflow/flow-js-testing/node_modules/@onflow/flow-cadut/src/imports.js:103:15)
      at replaceImportAddresses (node_modules/@onflow/flow-js-testing/src/interaction.js:95:14)
      at Object.<anonymous> (tests/hello-world.test.ts:33:25)

@adbario
Copy link
Contributor

adbario commented Sep 15, 2022

Also I'm able to successfully run all tests from flow-nft

@gregsantos Please note that the tests on flow-nft use 0.3.0-alpha.14 which works fine for me as well. The issues reported are for the latest version 0.3.0-alpha.15

To double check this, I upgraded the testing library on flow-nft and all the same errors come up.

@bluesign
Copy link

bluesign commented Sep 15, 2022

@gregsantos @adbario

This is a strange issue ( though was fun to debug), I don't know javascript much. ( so my analysis can be wrong )

Problem is here:

export const sendTransaction = async (...props) => {

export const sendTransaction = async (...props) => {
  let result = null,
    err = null
  const logs = await captureLogs(async () => {
    try {
      const extractor = extractParameters("tx")
      const {code, args, signers, limit} = await extractor(props)

      const serviceAuth = authorization()

here somehow props having problem passing sometimes ( or somehow overwritten )

changing something like:

export const sendTransaction = async (...props) => {
  let result = null,
    err = null
 let propsOuter = props
  const logs = await captureLogs(async () => {
    try {
      const extractor = extractParameters("tx")
      const {code, args, signers, limit} = await extractor(propsOuter)

      const serviceAuth = authorization()

solves the problem. Maybe some JS bug, packer bug, or something else. If someone knows the answer I am curious too

@jribbink
Copy link
Contributor

@bluesign is correct and this is yet another one of these strange microbundle issues related to async function arguments, very similar to previous occurances in @onflow/fcl. The bundled code looks like the following:

const sendTransaction = function () {
  try {
    let result = null,
        err = null;
    return Promise.resolve(captureLogs(function () {
      try {
        const _arguments = arguments;

        const _temp7 = _catch(function () {
          const extractor = extractParameters$1("tx");
          return Promise.resolve(extractor([].slice.call(_arguments))).then(function ({
            code,
            args,
            signers,
            limit
          }) {

Note that the arguments come from the inner function captureLogs and not the outer function sendTransaction as intended.

Using the internal build tool, @onflow/fcl-bundle, is not possible without a large amount of refactoring (see #152) because flow-js-testing has a lot of tricky circular dependencies. Microbundle is somehow able to ignore these, but fcl-bundle is not (my guess is something related to the use of the rollup babel plugin in fcl-bundle and this confuses the code transformations somehow).

@adbario
Copy link
Contributor

adbario commented Sep 18, 2022

Maybe some JS bug, packer bug, or something else. If someone knows the answer I am curious too

Javascript just never stops surprising us.

And yes, all the tests within the example folder are passing. But when the library is bundled that doesn't seem to be the case anymore.

@justinbarry
Copy link
Contributor

@jribbink Good call on the scoping issue. Was able to patch it in this case.

@sisyphusSmiling
Copy link
Contributor Author

Looks like this issue could be popping up again in #191. Cross posting here for visibility

@gregsantos
Copy link
Member

Fixed via #190

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
7 participants