Testing - No new node

[kaidatavis]

https://goo.gl/8zPGzT

1. Make sure you understand the behaviours described on the wiki page. Let me know if it is not clear or you don’t agree (for example you think this is not the best way to help user understand what happened);
2. Create (Jasmine) 'test specs’ for these behaviours (up to the ‘it’ level, but not the function inside it), following Phong’s folder structure; 1a: for the ‘unclear’ cases, just use the current SenseMap behaviour;
3. Add the actual javascript and sinon-chrome code to make the test running;
4. For each behaviour, create different types of testing instances. Using ‘new node - manually enters a URL (in the address bar)’ as an example, possible instance can be:
   • url for different websites
   • different protocols: http, https,
   • without protocol
   • invalid url
   • …
5. There is an extra challenge to figure out how to detect page redirection.

[shahzaibkhan]

@kaimdx basically you want to test the behavior when a new node is getting created?

For point no. 4 please explain a bit more on how you want it to be test.

[kaidatavis]

basically you want to test the behavior when a new node is getting created?

In your case, it is actually testing the cases when no sensemap node should be created. Currently there are two cases as described under the 'No new node' section: ignored url and redirection. The former is not difficult, but new code will be needed to detect redirection (we had a look of google api on redirection before).

For point no. 4 please explain a bit more on how you want it to be test. For example, test a number of different Chrome urls for the 'ignored url' case: extension page, setting page, new tab, etc.

[shahzaibkhan]

This is the case i am try to achieve, please review and let me know if it makes sense. I am checking for ignore urls and if those are found then it share us that its a no new node case.

describe("Case 1: No New node", function () { let caseNoNewNode = false; beforeAll(function(done) { const manualUrl = 'chrome://'; const ignoredUrls = [ 'chrome://', 'chrome-extension://', 'chrome-devtools://', 'view-source:', 'localhost://' ]; browser = sm.provenance.browser() .on('nodeCreated', function(action) { if (ignoredUrls.some(url => action.url.includes(url))) caseNoNewNode = true; }); chrome.tabs.create({ url: manualUrl, active: false }, function(tab) {});

});

it('should appear on the history map when a new url is amongst the ignoreurl', function() {
    expect(caseNoNewNode).toBeTruthy();
});

});

[shahzaibkhan]

When doing testing, this error seems to be returning:

Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

The code opens the node successfully, then on test area there is a certain delay and then it returns above error.

Code is here: describe('Case 1: No New node', function () {

let caseNoNewNode = false;

beforeAll(function(done) {

    var manualUrl = "https://chrome-extension://";
    browser = sm.provenance.browser()
        .on('nodeCreated', function(action) {

            if (action.url === manualUrl) {
                caseNoNewNode = true;
            } 

            if(caseNoNewNode) done();
        });

        chrome.tabs.create({ url: manualUrl, active: false }, function(tab) {

    });
});

it('Testing:` No new node should appear on the history map when a new url is amongst the ignoreurl', function() { expect(caseNoNewNode).toBeTruthy(); }); });

Kindly test why is this happening.

[kaidatavis]

Besides what I said in the other issue, I would say try to do the sinon testing first, which does not require async test. Once that is working, and then try using Jasmine only.

Have a look of the sinon test in the latest rebuild branch (node-sinon-chrome.test.js). It fixes a couple of issues, such as doing the check in the beforeAll, and checks the url/title/favcon (not just a function is ran once). Except the data object, the actual testing code is just one line.

[kaidatavis]

@shahzaibkhan good to see the progress. I can see you are making changes based on the discussions in last meeting, though they are not quite working yet. I can see you are following the latest way to test in the 'rebuild' branch, which is good. Let's discuss more at the meeting.

[shahzaibkhan]

Sounds Good. The issue is probably with the asyc testing. Definitely we can have discussion about it.

On Wed, Jul 12, 2017 at 3:41 PM, Kai Xu notifications@github.com wrote:

@shahzaibkhan https://github.com/shahzaibkhan good to see the progress. I can see you are making changes based on the discussions in last meeting, though they are not quite working yet. I can see you are following the latest way to test in the 'rebuild' branch, which is good. Let's discuss more at the meeting.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Vis4Sense/SenseMapExtension/issues/72#issuecomment-314724398, or mute the thread https://github.com/notifications/unsubscribe-auth/AAusWUgYLXzVzYtSi_NwIW0DIMid2CVBks5sNKLygaJpZM4N8pnh .

--

-- --

Thanks & Regards,

Shahzaib Khan

Email shahzaib198@gmail.com | Blog http://shahzaibkhan.com/ | Twitter http://twitter.com/shahzaib_khan | Facebook http://www.facebook.com/shahzaib198 | Linkedin http://www.linkedin.com/in/shahzaibkhan

This message is intended for the recipient named above. It may contain confidential or privileged information. If you are not the intended recipient, please notify the sender immediately by replying to this message and then delete it from your system. Any form of unauthorized use or dissemination is prohibited. Risks are inherent in all internet communication. Each recipient is responsible for protecting its system from viruses and/or other harmful code and/or device. The sender is not responsible, and hereby disclaims all liabilities arising from or in relation to any viruses and/or other harmful code and/or device.

[shahzaibkhan]

The test is now in place, please review. Updated rebuild-shaz branch.

[kaidatavis]

This one is almost there. The only change needed is to add a few cases for each type of url. For example for 'chrome://extensions/' it shoudl test URLs such as 'chrome://extensions/?id=gekbinhnlmhidijgdccmnakheaeplado' and 'chrome-extension://gekbinhnlmhidijgdccmnakheaeplado/src/html/options.html'. The applies to all five types of URLs.

[shahzaibkhan]

Do you mean i should add these urls for test?

On Tue, Jul 18, 2017 at 4:11 PM, Kai Xu notifications@github.com wrote:

This one is almost there. The only change needed is to add a few cases for each type of url. For example for 'chrome://extensions/' it shoudl test URLs such as 'chrome://extensions/?id=gekbinhnlmhidijgdccmnakheaeplado' and 'chrome-extension://gekbinhnlmhidijgdccmnakheaeplado/src/html/options.html'. The applies to all five types of URLs.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Vis4Sense/SenseMapExtension/issues/72#issuecomment-316032000, or mute the thread https://github.com/notifications/unsubscribe-auth/AAusWcDSRNJSpwmj_JyyvY9GlqbLfk9jks5sPJJ2gaJpZM4N8pnh .

--

-- --

Thanks & Regards,

Shahzaib Khan

Email shahzaib198@gmail.com | Blog http://shahzaibkhan.com/ | Twitter http://twitter.com/shahzaib_khan | Facebook http://www.facebook.com/shahzaib198 | Linkedin http://www.linkedin.com/in/shahzaibkhan

This message is intended for the recipient named above. It may contain confidential or privileged information. If you are not the intended recipient, please notify the sender immediately by replying to this message and then delete it from your system. Any form of unauthorized use or dissemination is prohibited. Risks are inherent in all internet communication. Each recipient is responsible for protecting its system from viruses and/or other harmful code and/or device. The sender is not responsible, and hereby disclaims all liabilities arising from or in relation to any viruses and/or other harmful code and/or device.

[kaidatavis]

For each of the five types of ignored URLs, add a few (real) examples in the test. The ones in my last comments are the examples for the URLs starting with 'chrome://extensions/'.

[shahzaibkhan]

K this is done. Added above cases as well. I have made it as a function, so you can test as many cases as you like. Already added alot of them.

nonownodetest("chrome-extension://"); nonownodetest("chrome-extension://sdsdsd"); nonownodetest("chrome-extension://?id=gekbinhnlmhidijgdccmnakheaeplado"); nonownodetest("chrome://"); nonownodetest("chrome://sdsdsd"); nonownodetest("chrome://?id=gekbinhnlmhidijgdccmnakheaeplado"); nonownodetest("chrome-devtools://"); nonownodetest("chrome-devtools://sdsdssd"); nonownodetest("chrome-devtools://?id=gekbinhnlmhidijgdccmnakheaeplado"); nonownodetest("localhost://"); nonownodetest("localhost://sdsdsddsd"); nonownodetest("view-source:"); nonownodetest("view-source:sdsdsdsd"); nonownodetest("view-source:?id=gekbinhnlmhidijgdccmnakheaeplado");

[shahzaibkhan]

Once tested, you can close this task : )

[kaidatavis]

Good to see the progress. The new urls make the testing more complete. However, to make the test a bit more robust and strict, please first check if the node array is empty or not. If it is, the current testing condition is fine (or just node array length is still 0). If the node array is not empty, the last node should be same after the onUpdated (this code is not in the test spec yet).

[shahzaibkhan]

@kaimdx this is already being test:

const lastNode = _.last(sm.data.tree.nodes); //fetches the last node expect(lastNode).not.toBeDefined(); //here it test: if it is not defined then the condition is met,

Plus if you try and check the length it will always come as 0.

[kaidatavis]

const lastNode = _.last(sm.data.tree.nodes); //fetches the last node expect(lastNode).not.toBeDefined(); //here it test: if it is not defined then the condition is met,

What if the node array is not empty at the start? Some other tests can add nodes to the array before this test.

[shahzaibkhan]

K that also is added.

[shahzaibkhan]

Its going to test against all conditions:

noNewNodeTest("chrome-extension://"); noNewNodeTest("chrome-extension://sdsdsd"); noNewNodeTest("chrome-extension://?id=gekbinhnlmhidijgdccmnakheaeplado"); noNewNodeTest("chrome://"); noNewNodeTest("chrome://sdsdsd"); noNewNodeTest("chrome://?id=gekbinhnlmhidijgdccmnakheaeplado"); noNewNodeTest("chrome-devtools://"); noNewNodeTest("chrome-devtools://sdsdssd"); noNewNodeTest("chrome-devtools://?id=gekbinhnlmhidijgdccmnakheaeplado"); noNewNodeTest("localhost://"); noNewNodeTest("localhost://sdsdsddsd"); noNewNodeTest("view-source:"); noNewNodeTest("view-source:sdsdsdsd"); noNewNodeTest("view-source:?id=gekbinhnlmhidijgdccmnakheaeplado"); noNewNodeTestWithDataPresent("chrome-extension://"); noNewNodeTestWithDataPresent("chrome-extension://sdsdsd"); noNewNodeTestWithDataPresent("chrome-extension://?id=gekbinhnlmhidijgdccmnakheaeplado"); noNewNodeTestWithDataPresent("chrome://"); noNewNodeTestWithDataPresent("chrome://sdsdsd"); noNewNodeTestWithDataPresent("chrome://?id=gekbinhnlmhidijgdccmnakheaeplado"); noNewNodeTestWithDataPresent("chrome-devtools://"); noNewNodeTestWithDataPresent("chrome-devtools://sdsdssd"); noNewNodeTestWithDataPresent("chrome-devtools://?id=gekbinhnlmhidijgdccmnakheaeplado"); noNewNodeTestWithDataPresent("localhost://"); noNewNodeTestWithDataPresent("localhost://sdsdsddsd"); noNewNodeTestWithDataPresent("view-source:"); noNewNodeTestWithDataPresent("view-source:sdsdsdsd"); noNewNodeTestWithDataPresent("view-source:?id=gekbinhnlmhidijgdccmnakheaeplado");

[kaidatavis]

This is good. I simplified the code a bit in the latest commit. Please update the testing so it compares the node array before and after, and make sure the two are exactly the same. Currently, it only checks the length, but the content of a node can be changed (while the node length remains the same). You can compare two arrays element by element to check if they are the same.

[shahzaibkhan]

K new update has been pushed.

It will also test now for before and after titles. expect(afterTitle).toEqual(beforeTitle);

[kaidatavis]

This is certainly the right direction, but not quite there yet. Please 1) check every attribute, not just 'title', is the same, and 2) do this for all the nodes in the array assuming the array length is not known.

[shahzaibkhan]

2) do this for all the nodes in the array assuming the array length is not known. -> we just add 1 node so obviously we expect in the test to have a single node?

[kaidatavis]

This will make the test more robust. It will still work if we later decide to include a more complicated case with many nodes in the array. Also, it is not that much work, mostly a loop.

[shahzaibkhan]

K Added the other attributes. Done.

[kaidatavis]

I can see you are testing more node attributes now, but it is still not quite 'every' attribute. Also, it is only in 'noNewNodeTestWithDataPresent', but not in 'noNewNodeTest'. Finally, I don't see the testing for array, as mentioned earlier:

2) do this for all the nodes in the array assuming the array length is not known.

[kaidatavis]

How about we do it this way: let's create two functions, one for checking if two 'nodes' are identical

1) check every attribute, not just 'title', is the same,

So it will be something like

function areNodesSame (n1,n2) {
   if (every attribute of n1 is the same as that in n2) return true;
   else return false;
}

The second function compares two node arrays:

2) do this for all the nodes in the array assuming the array length is not known.

So it will be something like

function areNodeArraysSame (ary1,ary2) {
   if (ary1.length !== ary2.length) return false;
   for (var i=0; i<ary1.length; i++) {
      if (!areNodesSame(ary1[i],ary2[i])) return false;
   }
   return true;
}

These functions can then be used in other testings, such as #79

[shahzaibkhan]

By every attribute can you clarify what items to include:

Title URL Favicon

And ?

Because rest of the other remains same since those are dependent upon the tab.

Sent from my iPhone

On 28-Jul-2017, at 2:59 PM, Kai Xu notifications@github.com wrote:

How about we do it this way: let's create two functions, one for checking if two 'nodes' are identical

check every attribute, not just 'title', is the same, So it will be something like

function areNodesSame (n1,n2) { if (every attribute of n1 is the same as that in n2) return true; else return false; } The second function compares two node arrays:

do this for all the nodes in the array assuming the array length is not known. So it will be something like

function areNodeArraysSame (ary1,ary2) { if (ary1.length !== ary2.length) return false; for (var i=0; i<ary1.length; i++) { if (!areNodesSame(ary1[i],ary2[i])) return false; } return true; } These functions can then be used in other testings, such as #79

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[kaidatavis]

Can you just do a loop go through every attribute of an object, so you don't need to know how many attributes there are and what they are beforehand?

[shahzaibkhan]

In some cases attribute don't even exist. Like favicon is present in some case while in otter it doesn't. So that is why it be very tough to test through all at once.

Sent from my iPhone

On 28-Jul-2017, at 9:47 PM, Kai Xu notifications@github.com wrote:

Can you just do a loop go through every attribute of an object, so you don't need to know how many attributes there are and what they are beforehand?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[kaidatavis]

It is possible to compare node properties without knowing what attributes they have. This example is a way to iterate through all the object properties without knowing the property name: https://stackoverflow.com/questions/8312459/iterate-through-object-properties

[shahzaibkhan]

Thanks for the notes, they helped. Have tested through which ever property is available. So loops through all. Hope that should be it. Please review.

[kaidatavis]

That's good progress, but the logic is not quite right yet. The test should fail if 'afterObj' doesn't have a 'property' from the 'beforeObj'.

[shahzaibkhan]

Ok yes was not considering the ones which are not coming. Anyways that we can allow to prompt as failure

Sent from my iPhone

On 31-Jul-2017, at 10:05 PM, Kai Xu notifications@github.com wrote:

That's good progress, but the logic is not quite right yet. The test should fail if 'afterObj' doesn't have a 'property' from the 'beforeObj'.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[shahzaibkhan]

K it now checks all the attributes.