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

fix: Made timestamp consistant #46

Merged
merged 10 commits into from
Aug 21, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/lib/ext-activitylog.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,8 @@ class Model {

const startTime = this.filter.timeStamp?.start;
const stopTime = this.filter.timeStamp?.stop;
const logTime = Date.parse(logTimestamp);

if (logTime < startTime || logTime > stopTime) {
if (logTimestamp < startTime || logTimestamp > stopTime) {
return false;
}

Expand Down
16 changes: 16 additions & 0 deletions src/lib/ext-listen.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,19 @@ export function openActivityLogPage() {
url: getActivityLogPageURL(),
});
}

export function dateTimeFormat(timestamp, options) {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you put this function in ext-listen.js instead of a new file called, say, formatters.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, made a new file formatters.js

const dateTime = new Date(timestamp);
const time = dateTime.toLocaleTimeString();
Copy link
Member

Choose a reason for hiding this comment

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

DateTimeFormat also supports parameters to include the time in the generated string. Do not use toLocaleTimeString, but only DateTimeFormat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :-)


if (options?.timeOnly) {
return time;
}

const dateFormatOptions = { month: 'short', day: 'numeric', year: 'numeric' };
const date = new Intl.DateTimeFormat(undefined, dateFormatOptions).format(
dateTime
);

return `${date} ${time}`;
}
3 changes: 3 additions & 0 deletions src/lib/ext-monitor.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ export default class ExtensionMonitor {

createLogListener() {
return async (details) => {
// set a timestamp(number) as the value of timeStamp property
details.timeStamp = Date.parse(details.timeStamp);
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the next comment:

// TODO: Stop using `Date.parse` when `details.timeStamp` is a numeric timestamp.
// See https://bugzilla.mozilla.org/show_bug.cgi?id=1660460


this.logs.push(details);
await this.sendLogs(details);
};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { dateTimeFormat } from '../../ext-listen.js';

export class FilterTimestamp extends HTMLElement {
constructor() {
super();
Expand Down Expand Up @@ -33,14 +35,15 @@ export class FilterTimestamp extends HTMLElement {

const timeStamp = this.timeStamp || {};

const chosenTimestamp = selectedRow.querySelector('.timestamp').textContent;
const chosenTimestamp = selectedRow._log.timeStamp;

if (info.menuItemId === 'startTime') {
timeStamp.start = Date.parse(chosenTimestamp);
this.startTimeLabel.textContent = chosenTimestamp;
timeStamp.start = chosenTimestamp;
this.startTimeLabel.textContent = dateTimeFormat(chosenTimestamp);
this.clearStartTimeBtn.hidden = false;
} else if (info.menuItemId === 'stopTime') {
timeStamp.stop = Date.parse(chosenTimestamp);
this.stopTimeLabel.textContent = chosenTimestamp;
timeStamp.stop = chosenTimestamp;
this.stopTimeLabel.textContent = dateTimeFormat(chosenTimestamp);
this.clearStopTimeBtn.hidden = false;
}

Expand Down
10 changes: 8 additions & 2 deletions src/lib/web-component/log-view/log-view-element.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { dateTimeFormat } from '../../ext-listen.js';

export class LogView extends HTMLElement {
constructor() {
super();
Expand Down Expand Up @@ -39,8 +41,12 @@ export class LogView extends HTMLElement {
logTableRowInstance.hidden = !this.isFilterMatched(log);

logTableRowInstance.querySelector('.id').textContent = log.id;
logTableRowInstance.querySelector('.timestamp').textContent =
log.timeStamp;

const timestamp = logTableRowInstance.querySelector('.timestamp');

timestamp.textContent = dateTimeFormat(log.timeStamp, { timeOnly: true });
timestamp.title = dateTimeFormat(log.timeStamp);

logTableRowInstance.querySelector('.api-type').textContent = log.type;

if (log.type === 'content_script') {
Expand Down
75 changes: 75 additions & 0 deletions tests/ext-activitylog.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import ActivityLog from '../src/lib/ext-activitylog';
import { FilterOption } from '../src/lib/web-component/filter-option/filter-option-element';
import { LogView } from '../src/lib/web-component/log-view/log-view-element';
import { FilterKeyword } from '../src/lib/web-component/filter-keyword/filter-keyword-element';
import * as ExtListen from '../src/lib/ext-listen';

const activityLogHtml = fs.readFileSync(
path.resolve(__dirname, '../src/activitylog/activitylog.html'),
Expand Down Expand Up @@ -269,3 +270,77 @@ test('clearing logs from activitylog page', async () => {
expect(activityLog.model.logs).toMatchObject([]);
expect(getTableRows().length).toBe(0);
});

test('timestamp is formatted and rendered correctly', () => {
const logTimestamp = 1597686226302;

const logs = [
{
/* renders 1st row in table */
id: 'id1@test',
viewType: 'viewType@test',
type: 'type@test',
data: [{ test: 'test1@data' }],
timeStamp: logTimestamp,
},
];

const addListener = jest.fn();
const sendMessage = jest.fn();
const connect = jest.fn();

window.browser = {
runtime: {
onMessage: { addListener },
sendMessage,
connect,
},
};

sendMessage.mockImplementation(() => {
return Promise.resolve({ existingLogs: [] });
});

document.body.innerHTML = activityLogBody;
const dateTimeFormatFn = jest.spyOn(ExtListen, 'dateTimeFormat');
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't mock your function, but the API that you're using. The only changes should be:

  • fix 'en-US' as the locale instead of undefined.
  • fix the timeZone option.

By doing both, the generated time strings become fully deterministic / predictable, and you can put the expected times in strings below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :-) Thanks


const { activityLog } = new ActivityLog();

// To have consistant date format, we choose "en-US" formatting
let expectedDate;
dateTimeFormatFn.mockImplementation((timestamp, options) => {
const dateTime = new Date(timestamp);
const time = dateTime.toLocaleTimeString();

if (options?.timeOnly) {
return time;
}

const dateFormatOptions = {
month: 'short',
day: 'numeric',
year: 'numeric',
};
expectedDate = new Intl.DateTimeFormat('en-US', dateFormatOptions).format(
dateTime
);

return `${expectedDate} ${time}`;
});

activityLog.handleNewLogs(logs);

expect(dateTimeFormatFn).toHaveBeenCalled();

const dateTime = new Date(logTimestamp);
const expectedTime = dateTime.toLocaleTimeString();
Copy link
Member

Choose a reason for hiding this comment

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

Hard-code the expected strings instead of generating it based on the dynamic objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :-)

const expectedDateTime = `${expectedDate} ${expectedTime}`;

const tableBody = activityLog.view.logView.shadowRoot.querySelector('tbody');
const tableRows = tableBody.querySelectorAll('tr');
const firstRowTimestamp = tableRows[0].querySelector('.timestamp');

// For log timestamp: 1597686226302
expect(firstRowTimestamp.textContent).toEqual(expectedTime);
expect(firstRowTimestamp.title).toEqual(expectedDateTime);
});