(fix) cursor reading didn't work in tests

[xavdid]

We were having an issue where cursors mostly didn't work in unit tests at all.

How it's supposed to work

When run as part of a zap, the server sends a storeKey that always points to the data that individual zap has previously saved (based on the node id). The most common pattern is that the first run sets a cursor and subsequent runs read that cursor, make a call, and set an updated cursor.

The problem

When running unit tests with a cursor, there isn't a node. Instead, we generated a random id so that multiple users wouldn't be fighting over keyspace. The issue is that separate invocations of appTest(App...) would create new random ids. Tests would write data to testKey-123 and then try to read from testKey-456 which was blank, causing the tests to fail.

The solution

The issue we're trying to avoid flaky tests caused by key collisions. If everyone's cursor wrote to key-cursor-test, then multiple people running tests within the same 60-second window (globally) would lead to weird behavior.

The first solution was using the method, but given that we rely so heavily on scaffolds and triggers like recipe, I was concerned we'd still have collisions. I wanted to use something unique to the user. Ideally we could use the app_id, but that doesn't always exist (eg: a new app), so username felt like a reasonable fallback. I felt weird about sending user's usernames to our server, so I figured hashing it was the best way to have the best of both worlds. Not perfect (there are still collisions on a per-user and per-method basis), but close enough.

Testing

Addresses PDE-273

I tested by linking core, and using the following trigger and test:

// recipe.js
const _ = require('lodash')

const randomStr = () =>
  Math.random()
    .toString(36)
    .slice(2)

// triggers on recipe with a certain tag
const triggerRecipe = async (z, bundle) => {
  const cursor = _.get(bundle, 'meta.page') ? await z.cursor.get() : null

  console.log('starting with', cursor)
  const newCursor = randomStr()
  await z.cursor.set(newCursor)

  return [{ id: 1, old: cursor, new: newCursor }]
}

module.exports = {
  key: 'recipe',
  noun: 'Recipe',

  display: {
    label: 'Get Recipe',
    description: 'Triggers on a new recipe.'
  },

  operation: {
    canPaginate: true,
    perform: triggerRecipe,

    sample: {
      id: 1
    }
  }
}

// test.js

it('should run triggers.recipe', async () => {
    const results = await appTester(App.triggers.recipe.operation.perform, {})
    console.log('results', results)
    should.exist(results)
    should(results[0].old).equal(null)
    should.exist(results[0].new)

    const results2 = await appTester(App.triggers.recipe.operation.perform, {
      meta: { page: 1 }
    })
    console.log('results', results2)
    should(results2[0].old).not.equal(null)
    should.exist(results2[0].new)

    results2[0].new.should.not.equal(results[0].new)
  })

[eliangcs]

I have a question on how you hash USERNAME and method path and a suggestion on where you place md5 function. Would like to get clarification before further reviewing. Thanks!

[eliangcs]

It's probably better to move this function to src/tools/hashing.js.

[eliangcs]

I'm still trying to figuring out how cursor works. Correct me if I'm wrong. What you want to do is to keep storeKey unique to the method invoked, correct? If so, can we keep it simple by setting storeKey to the method part? I.E.:

let storeKey = null;
if (shouldPaginate(appRaw, method)) {
  storeKey = method;
}

const event = {
  command: 'execute',
  method,
  bundle,
  storeKey
};

Doesn't USER or USERNAME stay constant across all the test cases? So I don't see the need for involving them into this. Let me know if I missed anything.

[xavdid]

I updated the OP (since comments always get lost). Let me know if it doesn't make sense

[eliangcs]

Really appreciate the detailed explanation! That does make a lot of sense. Since devs are hitting Zapier server when they run tests with z.cursor, we need a way to separate cursor storage by devs. But USER seems a bit fragile. Is it true that if two devs have the same username on their machines, they may collide with each other? Can we use a more unique thing? I was thinking about MAC address, but there doesn't seem to be a cross-platform way for Node.js to get it. How about we keep using genId()? But instead of calling getId() on every appTester(method, bundle) call, we only call genId() once when createAppTester is called. Here's the idea:

const createAppTester = appRaw => {
  const handler = createLambdaHandler(appRaw);
  const createHandlerPromise = promisifyHandler(handler);

  // A unique ID for this test run
  const testId = getId();  // probably better to use randomString() to be more unique

  return (methodOrFunc, bundle) => {
    bundle = bundle || {};

    const method = resolveMethodPath(appRaw, methodOrFunc);

    // we need a value that's unique to this test (but consistent across test runs) so we don't lose cursors
    const unique = md5(`${method}-${testId}`);

    const event = {
      command: 'execute',
      method,
      bundle,
      storeKey: shouldPaginate(appRaw, method) ? `testKey-${unique}` : null
    };

    // ...
  };
};

[xavdid]

Ah, that'll do it! I worked myself into a loop at made it a lot more complex than it needed to be.

Also FWIW, collision risk here would have only been for users with identical usernames and test methods who run their cursor tests within 60 seconds of each other. not zero, but low enough.

In any case, I think your solution is better! I've pushed that change.

[eliangcs]

👍