Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
    Asko Soukka
    @datakurre
    We already have high level acceptance tests for all the old features. I take the responsibility for upgrading that to support testing changes through websocket events. (That would be mainly bash scripting and Robot Framework, so for you that would be quite a step outside current JavaScript focus.)
    Most obvious choice would be to keep moving code from gatsby-node.js to utils.js or some other module and test those funtions with jest unit tests.
    But integration tests for the GatsbyJS API hooks in gatsby-node.js are still missing and there I would like GatsbyJS to provide some example or framework to do it right (and easily).
    Then again, another way for better quality would be to convert the project into typescript. It would probably have larger impact than having integration tests, but the amount of work in that is hard to estimate.
    @iFlameing So, because this is now only the first week of the second part of GSOC, I think it would be good to use a week to just learn the other plugins and then decide. If you run out of work, look into typescript also. This seemed to be a plugin using it https://github.com/sanity-io/gatsby-source-sanity
    Alok Kumar
    @iFlameing
    @datakurre okay! I will see the test written for different Gatsby-preview CMS and interact with GatsbyJS community for How we can write test for GatsbyJS API :)
    Asko Soukka
    @datakurre
    @iFlameing Thanks! After this week I would expect you to have an opinion on which way would make most sense (more testing, which kind of testing or just typescript).
    Alok Kumar
    @iFlameing
    @datakurre I will give my best In researching on these topics and develop my opinion about these different options.
    Asko Soukka
    @datakurre
    Great! Thanks!
    Alok Kumar
    @iFlameing
    @datakurre I read the test written for utils.js. I also read the documentation of jest. So, in my opinion we should write unit test first because it confirms that our logic is correct. Whether we use typescript we need to write unit test because typescript only ensure that whatever we returning from the function is of correct form but it doesn't ensure that our logic is correct. I asked Jason Lengstorf for integration tests for the GatsbyJs API and he replied "Each of the Gatsby APIs is a function, so you can unit test by importing and stubbing out Gatsby helpers. For integration testing, it would be easier to test the output (e.g. ensure pages were created by createPages). " I don't think I understand it fully. what should I do now?
    Asko Soukka
    @datakurre
    @iFlameing Thank you for your research.
    @iFlameing We do already have acceptance tests to test that pages are created and I'm on my way to extend that approach to also support testing that your code creates, updates and deletes page on "gatsby develop".
    @iFlameing So I can agree with you that focusing on unit tests might be the best next step.
    Asko Soukka
    @datakurre
    Possibly you should add gatsby-node.tests.js here https://github.com/collective/gatsby-source-plone/tree/master/src/__tests__ and start trying to unit-test functions, starting from the seemingly easiest ones.
    This should be interesting reference test case https://github.com/collective/gatsby-source-plone/blob/f332e32784e15609d8d0d488cbb3d62a25089138/src/__tests__/utils.tests.js#L90
    To be able to test that fetchPlone fetches all pages from paged REST API endpoint, I needed to refactor fetchPlone to take the used HTTP client library as an argument. As seen in utils.js it defaults to the imported library, but for testing it allows to pass a mock library, to test the function without making actual HTTP calls.
    mocking = stubbing
    Asko Soukka
    @datakurre
    Similar approach should work for testing all functions, but usually requires touching the function to make them accept the mock/stub as an argument.
    What could a stub be for a GatsbyJS helper? E.g. something that simply counts how many times it has been called and then the test assert just checks that it has been called correct amount of times.
    Asko Soukka
    @datakurre
    Meanwhile I did update versions in the master branch. Changes in GatsbyJS required to replace the use of yarn link with manually symlinking the plugin into test project's node_modules. If you face strange issues, a fresh clone should help. They added some GatsbyJS themes related autodiscovery that broke yarn linked plugins, because GatsbyJS auto-discovers iteself from linked plugin's node_modules...
    To summarize:
    • Try unit-testing functions in src/gatsby-node.js in src/tests/gatsby-node.tests.js
    • I will update acceptance tests to test the new code (and that mostly covers integration tests)
    • If you have issues with master due to my changes, try a fresh clone
    I'm sorry that I'm online weird hours now in July. I'm actually on vacation and I'm mostly on keyboard when the rest of my family is already asleep.
    Alok Kumar
    @iFlameing
    @datakurre thanks for explaining everything in so details.

    I'm sorry that I'm online weird hours now in July. I'm actually on vacation and I'm mostly on keyboard when the rest of my family is already asleep.

    That’s absolutely fine :)

    Asko Soukka
    @datakurre

    @iFlameing To make some heavy manual testing of your work:

    Leave "make watch" running and run "make init-backend" to trigger a lot of content updates. Delete all content from Plone and then start again with "make init-backend" to re-create everything.

    Alok Kumar
    @iFlameing
    @datakurre ok! I am trying that right now and update you with the result.
    Asko Soukka
    @datakurre

    I'm once again working on top-level acceptance tests for your work and I'm seeing those random errors that might actually be GatsbyJS issues. I haven't properly reproduced those yet.

    Those might also be issues caused by gatsby-source-plone asynchronously processing multiple conflicting notifications.

    I wonder if we should wrap notification handlers with a lock https://www.npmjs.com/package/async-lock

    @iFlameing Are you familiar with concept of locks (usually familiar from threaded programming)?
    Alok Kumar
    @iFlameing
    @datakurre No, But I will learn
    Asko Soukka
    @datakurre

    @iFlameing Ok. I believe the README of https://www.npmjs.com/package/async-lock explains it well.

    I've never used that package before, but I've neither ever used locks with JavaScript, so that simple the first Google result.

    @iFlameing I believe that currently every "await" within a single handling of ws.onmessage allows NodeJS to switch execution context.

    For example, whenever we are waiting for the results of call to search-endpoint (for example, to update breadcrumbs or navigation), NodeJS is allowed to start processing next ws.onmessage event.

    Alok Kumar
    @iFlameing
    @datakurre I am thinking that it works like this if any async function is running and if we call it again we have to wait for earlier function call to complete. what do you think?
    Asko Soukka
    @datakurre
    This is just a sophisticated guess, but that may lead to unexpected order of updating / deleting / creating GatsbyJS nodes.
    @iFlameing Yes, I believe. So if we would wrap every ws.onmessage-handler code with async-lock "acquire", only one ws.onmessage would be handled at time.
    Alok Kumar
    @iFlameing
    @datakurre I am currently writing a test for breadcrumbs node. I will make a pr tomorrow and start exploring with the async-lock npm and without it.
    Asko Soukka
    @datakurre
    @iFlameing Thanks. Obviously you should finish your current test tasks first, because every new test protects from regression.
    Alok Kumar
    @iFlameing
    @datakurre I am able to complete these test earlier but writing test for makecontentNode takes lot's of time
    I have to read lot's of code and understand it to make the mock data :)
    Asko Soukka
    @datakurre
    No problem. I'm happy that we were able to use a complete GSOC phase for testing.
    Btw, when I get some new acceptance tests merged I will also rename folder "tests/gatsby-starter-default" to just "example".
    Alok Kumar
    @iFlameing
    @datakurre can we name something like demo because It is more suitable and Gatsby community also following this?
    Asko Soukka
    @datakurre
    @iFlameing Ok. I'll name it "demo" then if that's used elsewhere.
    Anything is better than the current :)
    Alok Kumar
    @iFlameing
    @datakurre In the recent release of Gatsby theme, they named most of them as demo .
    Asko Soukka
    @datakurre
    @iFlameing Thanks for following that.
    Alok Kumar
    @iFlameing
    @datakurre I am going to bed because it is around 1:30 am in my timezone. gdnight :)
    Asko Soukka
    @datakurre
    Good night!
    Asko Soukka
    @datakurre

    I have now added some acceptance tests that do "black box testing" that your GSOC additions update gatsby develop when pages are deleted, modified or added.

    Example run starting from line 1222 at output log of https://travis-ci.org/collective/gatsby-source-plone/jobs/561341455

    MVP code is a bit noisy, but hopefully you have time to refactor and cleanup the code in the last part of GSOC once all planned unit tests have been written.
    Tests in question are here but (in my opinion) that test setup is outside the scope of this GSOC https://github.com/collective/gatsby-source-plone/blob/master/demo/test-updates.robot#L58