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

converted editor_spec test to jest test fixes #772 #773

Merged
merged 8 commits into from
Jan 11, 2022

Conversation

NARUDESIGNS
Copy link
Collaborator

Hi guys, trust you had fun during the holidays.
This is a PR in progress but here's some information I discovered during the process.

  1. page.evaluate() function returns undefined for evaluations that are not asynchronous so during the test, jest receives undefined and not the evaluated value. To solve this I used page.evaluateHandle() which returns the evaluated value if the evaluation is not an asynchronous process. More info here
    I'm open to suggestions on how to handle this if there's a better way but currently, this works just fine.
  2. One of the test case from the editor_spec.js jasmine file includes a test instance expect(editor.options.textarea).toBe($('.ple-textarea')[0]) which usually returned false when I tried to convert it to jest test because those both comparing factors returns a value that is a reference data type. In javascript, {state: true} is not equal to {state: true}. This, I think is the problem so currently, I've commented it out. Maybe I can compare their keys or values instead.

@jywarren @TildaDares

@gitpod-io
Copy link

gitpod-io bot commented Jan 3, 2022

@NARUDESIGNS
Copy link
Collaborator Author

Also, the unfinished part is the part where an Ajax call was made. It looks like this:

  it("sends AJAX request on editor.publish()", function(done) {
    jasmine.Ajax.install();

    editor.options.destination = '/post';

    var ajaxSpy = spyOn($, "ajax").and.callFake(function(options) {
      if (options === editor.options.destination) {
        // http://stackoverflow.com/questions/13148356/how-to-properly-unit-test-jquerys-ajax-promises-using-jasmine-and-or-sinon
        var d = $.Deferred();
        d.resolve(options);
        d.reject(options);
        return d.promise();
      }
    });

    function onPublish(response) {
      expect(response).not.toBeUndefined();

      jasmine.Ajax.uninstall();
      done();
    }

    editor.publish(onPublish);
  });

I'm yet to figure out how to do it in puppeteer but I'm on it anyway. Any suggestion or guidance is welcome.

@jywarren
Copy link
Member

jywarren commented Jan 3, 2022

Hi Paul - so this is called "mocking" - that is, pretending to receive a response from a server during tests. Here's a good guide for "mocking a jQuery AJAX call":

https://stackoverflow.com/questions/44571207/mocking-jquery-ajax-with-jest

Background on Jest Mock functions: https://jestjs.io/docs/mock-functions

@NARUDESIGNS
Copy link
Collaborator Author

Hi Paul - so this is called "mocking" - that is, pretending to receive a response from a server during tests. Here's a good guide for "mocking a jQuery AJAX call":

https://stackoverflow.com/questions/44571207/mocking-jquery-ajax-with-jest

Background on Jest Mock functions: https://jestjs.io/docs/mock-functions

OK thank you, I'd check it out.

@jywarren
Copy link
Member

jywarren commented Jan 4, 2022

OK, so i think we could just do this the really basic way, by manually mocking $.ajax() --

$.ajax(
_editor.options.destination,
{
data: formatted,
method: 'POST'
}
).done(function(response) {
if (callback) callback(response);
else window.location = response;
}).fail(function(response) {
$('.ple-publish').removeClass('btn-success')
.addClass('btn-danger');
});

Here is the code inside of editor.publish().

We could just make a new function inside the evaluate block like:

const $ = {
  ajax: function(options) {
        var d = $.Deferred(); // this is just creating a promise, i think. Maybe we could do it manually without jQuery, but this could also just work. 
        d.resolve(options);
        d.reject(options);
        return d.promise(); // this so that when the Editor code runs .done() and .fail() on the response, those are run on the promise.
  }
}

That would override jQuery's $.ajax() function but return a promise nonetheless.

@jywarren
Copy link
Member

jywarren commented Jan 4, 2022

Try writing out this test and I can make suggestions inline!

@NARUDESIGNS
Copy link
Collaborator Author

Try writing out this test and I can make suggestions inline!

@jywarren but this line below is still trying to access $ which we already set as a constant variable.
var d = $.Deferred();

But I came across this right here which is jest's way of mocking calls.

I tried this below and the test passed but I don't know why it passes, I was expecting it to throw an error for the $ symbol

let spy = jest.fn().mockImplementation(async (options) => {
     await page.evaluate(() => {
        if(options === editor.options.destination) {
          let d = $.Deferred();
          d.resolve(options);
          d.reject(options);
          return d.promise();
        }
      });

      function onPublish(response) {
        expect(response).toBeDefined();
      }
  
      editor.publish(onPublish);
    });
  });

@NARUDESIGNS NARUDESIGNS changed the title converted editor_spec test to jest test *unfinished* fixes #772 converted editor_spec test to jest test fixes #772 Jan 10, 2022
@jywarren
Copy link
Member

Just catching up here -- why did you expect the error for the $ symbol and on which line?

compare their keys or values instead.

I like this. Is this what you ended up doing? Thanks!

const spy = jest.fn().mockImplementation(async (options) => {
await page.evaluate(() => {
if (options === editor.options.destination) {
const d = $.Deferred();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here you expected it not to be able to use $? I think it has access within the page context! Which is great.

expect( await page.evaluate(() => document.querySelector('.ple-textarea')) ).toBeDefined();
expect( await page.evaluateHandle(() => editor) ).toBeDefined();
expect( await page.evaluateHandle(() => editor.options.textarea) ).toBeDefined();
expect( await page.evaluateHandle(() => editor.options.textarea.innerHTML) ).toEqual(await page.evaluateHandle(() => document.querySelector('.ple-textarea').innerHTML));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like comparing strings worked here, re: expect(editor.options.textarea).toBe($('.ple-textarea')[0]) from your initial comment? I think this test looks good to go!

@jywarren jywarren merged commit 6e10b32 into publiclab:main Jan 11, 2022
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

Successfully merging this pull request may close these issues.

2 participants