-
Notifications
You must be signed in to change notification settings - Fork 147
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
persistancy test for store() method. #30
Conversation
frknbasaran
commented
Sep 17, 2018
- store(cly_queue), store(cly_event) and store(cly_id) variables tested for persistancy.
- Countly.test_mode property added into web sdk for prevent eventQueue emptying process when events moved into requestQueue.
- store(cly_queue), store(cly_event) and store(cly_id) variables tested for persistancy. - Countly.test_mode property added into web sdk for prevent eventQueue emptying process when events moved into requestQueue.
- cly_ignore, cly_token, cly_old_token, cly_cmp_id, cly_cmp_uid - counlty.js file called sync in html. - some controls deactived in test_mode.
- new tests added for check non-json values in storage. - Countly.device_id variable using now instead of store('cly_id') when reloaded page.
- events and other things removed from html. - add_event and other methods which created data on storage moved into first load of casperjs. - several test_mode controls added into countly web sdk, for hold eventQueue and requestQueue values.
lib/countly.js
Outdated
Countly.test_mode = Countly.test_mode || false; | ||
// if test mode enabled; update exposed objects too | ||
if (Countly.test_mode) Countly._internals.eventQueue = store("cly_event") || []; | ||
if (Countly.test_mode) Countly._internals.requestQueue = store("cly_queue") || []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we still need this ones?
lib/countly.js
Outdated
if (Countly.test_mode) { | ||
store('cly_cmp_id', 'test_campaign_id'); | ||
store('cly_cmp_uid', 'test_campaign_uid'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need this too, let's try to make lib as small as possible :)
So these values can be passed by URL parameters when you load it in casperjs like test.html?cly_cmp_id=test_campaign_id&cly_cmp_ui=test_campaign_uid
lib/countly.js
Outdated
if (!Countly.test_mode) { | ||
eventQueue = []; | ||
store("cly_event", eventQueue); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we need this too, as I dont think you test this method. Again just to make this lib as small as possible
lib/countly.js
Outdated
// if test mode enabled; push new event into exposed eventQueue array too | ||
if (Countly.test_mode) { | ||
Countly._internals.eventQueue.push(e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unused too now, isn't it?
lib/countly.js
Outdated
// disable this controls if test_mode is active | ||
if (!Countly.test_mode) { | ||
//ignore bots | ||
if (Countly.ignore_visitor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we just need one single check here
if(!Countly.ignore_visitor || Countly.test_mode)
and nothing more to checking test mode really needed?
- cly_id and cly_uid values passed as query params. - test_mode usages count reduced. - used getter methods for access eventQueue and requestQueue, unnecessary sync lines removed.
lib/countly.js
Outdated
@@ -491,8 +495,6 @@ | |||
//empty event queue | |||
if (eventQueue.length > 0) { | |||
toRequestQueue({ events: JSON.stringify(eventQueue) }); | |||
eventQueue = []; | |||
store("cly_event", eventQueue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, if there was a misunderstanding, but we need to empty queue on change_id
, what I meant was, you dont use it in tests, so no need to do anything test specific here :)
lib/countly.js
Outdated
if (Countly.ignore_visitor) { | ||
|
||
// disable this controls if test_mode is active | ||
if (!Countly.test_mode && Countly.ignore_visitor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this one, as it would prevent data from entering requestQueue
at all, no?
- current values compared with hardcoded values.