Vis4Sense / HistoryMap

http://sensemap.io
60 stars 7 forks source link

Testing - URL redirection #79

Closed shahzaibkhan closed 6 months ago

shahzaibkhan commented 7 years ago

Just to update:

  1. Have tried using Jasmine for this testing, logic that i have used was simple:

a. I am giving a X url and then i dont expect the same url to return, so if they dont match then case of redirection is true.

Code is here:

describe('Case 2: Redirection Test', function () {

    let caseRedirection = false;

    beforeAll(function(done) {

        var manualUrl = "http://movies.disney.com/finding-nemo";
        browser = sm.provenance.browser()
            .on('nodeCreated', function(action) {

                console.log(action.url);
                if (action.url !== manualUrl) {
                   caseRedirection = true;
                } 

                if(caseRedirection) done();

            });

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

        });
    });

it('Testing: Redirection of URL from one to other', function() {
    expect(caseRedirection).toBeTruthy();
    });
});

I seems to get error though its pretty straight forward logic: Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

kaidatavis commented 7 years ago

a. I am giving a X url and then i dont expect the same url to return, so if they dont match then case of redirection is true.

Yes, this is correct. It will be even better if you check if the node url is the one expected after redirection, not just the two URLs are different.

I seems to get error though its pretty straight forward logic: Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

This seems to be related to async testing in Jasmine. Can you check the part related to 'done' is correct?

shahzaibkhan commented 7 years ago

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

kaidatavis commented 7 years ago

Good work! This is certainly the right direction and it is good to see it is working. The issue is that it will need at least 2 'loading' event to simulate the 'redirection: the first one sends the 'urlstart' and the second one sends the 'urlend'. Currently 'urlstart' is not sent, so there won't be any node with 'urlstart'.

kaidatavis commented 7 years ago

The check needs to be more strict: 1) only one node is added (not two), 2) the url/title/fav of the new node match those in the right 'tab' object' (not just different).

kaidatavis commented 7 years ago

There is also the case I mentioned in #68. There need to be at least one more tabInfo sequence that have 'loading', 'complete', and 'loading' again. This will fail with the current implementation as the sensemap can't detect it as a redirection yet.

shahzaibkhan commented 7 years ago

Yup, either we pass twice the information to test or we just use the start and end url to verify, thoughts?

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

There is also the case I mentioned in #68 https://github.com/Vis4Sense/SenseMapExtension/issues/68. There need to be at least one more tabInfo sequence that have 'loading', 'complete', and 'loading' again. This will fail with the current implementation as the sensemap can't detect it as a redirection yet.

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

--

-- --

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 commented 7 years ago

Just copy all the update events from a real redirection such as google search result and send them one by one in the test using 'chrome.tabs.onUpdated.dispatch', so the code will look like:

save a copy (or the length) of the node array;

chrome.tabs.onUpdated.dispatch(loading event 1);
chrome.tabs.onUpdated.dispatch(loading event 2);
chrome.tabs.onUpdated.dispatch(title event);
chrome.tabs.onUpdated.dispatch(complete event);
chrome.tabs.onUpdated.dispatch(favUrl event);

check if the node array increases by one;
check if the new node url is the same as the url in loading event 2;
check if the new node title is the same as the title in the title event;
check if the new node favUrl is the same as the title in the favUrl event;

The test passes if all these conditions are true.
shahzaibkhan commented 7 years ago

So i have used this case:

cases

But when i try test the count it results into increase in the no of count of array: count

Code can be checked via: https://github.com/Vis4Sense/SenseMapExtension/blob/rebuild-shaz/tests/shaz/specs/redirection-sinon-chrome.test.js

Does this case has issue?

shahzaibkhan commented 7 years ago

Logically it should be 1 as a count

kaidatavis commented 7 years ago

It seems that you are not using the latest version of browserProvenance.js from the rebuild branch. Please update.

kaidatavis commented 7 years ago

Also, when it is good to test the current event sequence (4 'loading' events + 1 'complete' event), please also test the actual event sequence when typing in the chrome "http://movies.disney.com/finding-nemo". This is what I meant in the example earlier, with two 'loading' events, followed by 'title', 'complete', and 'favUrl'.

shahzaibkhan commented 7 years ago

@kaimdx:

  1. I used firstly use the chrome to fetch the actual LIVE sequence and i got 5 Loading events and 1 complete.
  2. Then implemented in the form of code.

Still after the update of browserProvenance.js file, i get same 2 array count.

kaidatavis commented 7 years ago

I used firstly use the chrome to fetch the actual LIVE sequence and i got 5 Loading events and 1 complete.

This is a bit weird. I got a different set of event when opening the url 'http://movies.disney.com/finding-nemo'

screen shot 2017-07-25 at 11 36 33

kaidatavis commented 7 years ago

Still after the update of browserProvenance.js file, i get same 2 array count.

This is indeed a bug (exactly what testing is for :-). This is fixed in the latest rebuild-shaz commit.

kaidatavis commented 7 years ago

Current the test spec does not have any 'expect', i.e., it doesn't do any actual testing yet.

Also, to improve the test spec, put the 'tabInfo' objects into an array and pass the array to the 'redirectionTest' function as a parameter.

shahzaibkhan commented 7 years ago
  1. It is not necessary that we will get the same flow every time, because each user has its own speed. And each user load time can vary so as the output from their SERVER can vary.

  2. Is the bug fixed?

  3. yes that right i didnt wrote expect yes since i was more keen in counting the array first.

kaidatavis commented 7 years ago

It is not necessary that we will get the same flow every time, because each user has its own speed. And each user load time can vary so as the output from their SERVER can vary.

The strange part is that we had different events. In your case it is 5 'loading' and a 'complete', no 'title' or 'favUrl', whereas in my case it is 'loading', 'title', 'loading', 'favUrl', 'title', and 'complete'. I don't think this relates how fast the server or internet is; the sequence should be the same even it takes a bit longer. We need to be careful and this can break the code logic, i.e., code works on a PC might not work on another. We need to collect these different sequences and add them to the test.

kaidatavis commented 7 years ago

Is the bug fixed?

Yes. Just check out the latest commit and test it.

kaidatavis commented 7 years ago

yes that right i didnt wrote expect yes since i was more keen in counting the array first.

I saw you made a new pull request, which indicates that the test code is done. Please ignore this if you are still working on the test code, which is not quite finished yet.

shahzaibkhan commented 7 years ago

So array is no 1 - problem solved.

Another problem that arise is that when retrieving the information, the first and last url should be changed but its not getting updated.

Firsr url passed is http://movies.disney.com/finding-nemo Last url passed is http://www.disneyinternational.com/

But it is still showing the first one: http://movies.disney.com/finding-nemo

OK here is another issue: image

Kindly look into this.

kaidatavis commented 7 years ago

This is a bug. Currently there is no url update after redirection is detected. I added that and committed the changes.

kaidatavis commented 7 years ago

I deleted the files with the old name convention, such as 'history-map-page.js'. You will need to update the links to these files to get the test running.

shahzaibkhan commented 7 years ago

OK new update is pushed, it now passes the test. Bug is removed. Please check.

kaidatavis commented 7 years ago

Good work. Similar to #72, can you check all the attribute and not just the 'url'? This is make the testing more strict.

shahzaibkhan commented 7 years ago

K Added. Done.

shahzaibkhan commented 7 years ago

K more attributes checking added. Please review.

kaidatavis commented 7 years ago

I don't see any change for the redirection testing.

shahzaibkhan commented 7 years ago

K it now checks all the attributes.

kaidatavis commented 7 years ago

Please make this a function, and use it to compare two arrays. It will be good if you can put these two functions (object compare and array compare) in a separate file so other tests can use them, too.