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

Test results are not consistent #45

Open
zrrrzzt opened this issue Jan 14, 2018 · 4 comments
Open

Test results are not consistent #45

zrrrzzt opened this issue Jan 14, 2018 · 4 comments

Comments

@zrrrzzt
Copy link
Contributor

zrrrzzt commented Jan 14, 2018

I did a PR and discovered one of the tests failed on travis. I ran the tests on my local machine and it passed. By running the tests multiple times I found that the result was not predictable.

The problem seems to be this line: https://github.com/PsychoLlama/gun-level/blob/master/src/spec.js#L87

How to recreate

clone repo
npm install

Add a script to the root of the repo run-multiple.js

const { exec } = require('child_process')
let jobs = 100
let done = 0
let fail = 0
let success = 0

function printResult () {
  console.log(`Fail: ${fail}`)
  console.log(`Success: ${success}`)
  console.log('\n')
}

while (jobs > done) {
  done++
  console.log(`Running ${done} of ${jobs}`)
  exec('npm test', (error, stdout, stderr) => {
    if (error) {
      fail++
    } else {
      success++
    }
    printResult()
  })
}

Run the script node run-multiple.js

Look at the results.

@amark
Copy link
Collaborator

amark commented Jan 14, 2018

I've also added my thoughts (should've posted them here, accidentally posted them in the other thread #44 ).

@zrrrzzt I am very grateful at how thorough and effective at testing/assessing/analyzing things!

Ideally, I'm gonna MERGE your PR and still consider this issue as open that we need to fix.

@amark
Copy link
Collaborator

amark commented Jan 14, 2018

Just FYI @zrrrzzt https://gitter.im/amark/gun?at=5a5bb8075ade18be397611e4 :) :) :) will add you to the README of contributors when I update the README next, too! I (and everybody else) are very grateful. :)

@zrrrzzt
Copy link
Contributor Author

zrrrzzt commented Jan 15, 2018

I think the right move would be to remove this test from gun-level if the adapter don't have anything to do with the merging in GUN. The tests should only test for the adapters responsibilities and internal functions not for GUN.

@zrrrzzt
Copy link
Contributor Author

zrrrzzt commented Jan 15, 2018

More updates. I tried run-multiple.jswith GUN v0.9.8still a lot of errors, but from the 4-5 I ran I would say there where fewer than with previous versions (best 48% success).

However. I created 2 other scripts merge-test.js for vanilla GUN and a script runner run-multiple.js With vanilla GUN (local and file) there where NO errors.

// merge-test.js
module.exports = callback => {
  const Gun = require('gun')
  const gun = Gun()
  const key = Math.random().toString(36).slice(2)
  // write initial data
  gun.get(key).put({ data: true }, res1 => {
    if (res1.err) {
      return callback(res1.err, null)
    }

    // add to it
    gun.get(key).put({ success: true }, res2 => {
      if (res2.err) {
        return callback(res2.err, null)
      }

      // verify data merge
      gun.get(key).val(value => {
        if (value.success && value.success === true && value.data && value.data === true) {
          return callback(null, {value: true})
        } else {
          return callback(null, {value: false})
        }
      })
    })
  })
}
//run-multiple.js
let jobs = 100
let done = 0
let fail = 0
let success = 0

function printResult () {
  console.log(`Fail: ${fail}`)
  console.log(`Success: ${success}`)
  console.log('\n')
}

function doTest () {
  const mergeTest = require('./merge-test')
  mergeTest((error, result) => {
    if (error) {
      console.error(error)
    } else {
      if (result.value === true) {
        success++
      } else {
        fail++
      }
      printResult()  
    }
  })
}

while (jobs > done) {
  done++
  console.log(`Running ${done} of ${jobs}`)
  doTest()
}

to run

node run-multiple.js

So there might still be something fishy with gun-levelhere

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

No branches or pull requests

2 participants