status-im
/
sourcecred
mirror of https://github.com/status-im/sourcecred.git
2
0
Fork 0
Code Issues Projects Releases Wiki Activity

Make TimestampMs validation optional (#1789) 

Currently, the Timestamp module requires that all TimestampMs types be
validated via the `fromNumber` method. However, as discussed in #1788,
this creates marginal benefit (just knowing that a TimestampMs is an
integer does not give great confidence that it's a meaningful and
correct timestamp for any use case), and creates a lot of overhead,
especially in test code. It makes adopting the TimestampMs type across
the codebase an arduous and mostly thankless task that would involve
re-writing dozens or hundreds of test instances to add zero actual
safety.

Therefore, I've made the type non-opaque, and renamed `fromNumber` to
`validate`. I then changed existing test code to not use the validator,
since we don't need any manual validation of our test cases, and `123`
is not a meaningful timestamp in any case :)

Test plan: `yarn test`
This commit is contained in:
Dandelion Mané 2020-05-13 11:44:23 -07:00 committed by GitHub
parent 96c34e3c6c
commit 34bb471d38
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 36 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
src/analysis/output.js
View File

@ -9,7 +9,6 @@ import {NodeAddress} from "../core/graph";
import type {Alias} from "../plugins/identity/alias";
import type {PluginDeclaration} from "./pluginDeclaration";
import type {TimestampMs} from "../util/timestamp";
import * as Timestamp from "../util/timestamp";
import {TimelineCred} from "./timeline/timelineCred";
import {nodeWeightEvaluator} from "../core/algorithm/weightEvaluator";
@ -66,14 +65,12 @@ export function fromTimelineCredAndPlugins(
// know what period to mint it in.
// When we transition to CredRank, we should remove this check.
const minted = timestampMs == null ? 0 : nodeEvaluator(address);
const timestamp =
timestampMs == null ? null : Timestamp.fromNumber(timestampMs);
return {
address: NodeAddress.toParts(address),
cred,
minted,
description,
timestamp,
timestamp: timestampMs,
};
}
);

7
src/plugins/initiatives/createGraph.test.js
View File

@ -7,7 +7,6 @@ import {
NodeAddress,
} from "../../core/graph";
import * as Weights from "../../core/weights";
import * as Timestamp from "../../util/timestamp";
import sortBy from "../../util/sortBy";
import type {ReferenceDetector, URL} from "../../core/references";
import {createWeightedGraph, initiativeWeight} from "./createGraph";
@ -30,7 +29,7 @@ import {
const exampleEntry = (overrides: $Shape<NodeEntry>): NodeEntry => ({
key: overrides.title ? _titleSlug(overrides.title) : "sample-title",
title: "Sample title",
timestampMs: Timestamp.fromNumber(123),
timestampMs: 123,
contributors: [],
weight: 456,
...overrides,
@ -40,7 +39,7 @@ function _createInitiative(overrides?: $Shape<Initiative>): Initiative {
return {
id: createId("UNSET_SUBTYPE", "42"),
title: "Unset test initiative",
timestampMs: Timestamp.fromNumber(123),
timestampMs: 123,
completed: false,
contributions: {urls: [], entries: []},
dependencies: {urls: [], entries: []},
@ -66,7 +65,7 @@ class MockInitiativeRepository implements InitiativeRepository {
const initiative = _createInitiative({
id: createId("TEST_SUBTYPE", String(num)),
title: `Example Initiative ${num}`,
timestampMs: Timestamp.fromNumber(400 + num),
timestampMs: 400 + num,
...shape,
});

11
src/plugins/initiatives/edgeSpec.test.js
View File

@ -1,7 +1,6 @@
// @flow
import {type TimestampMs} from "../../util/timestamp";
import * as Timestamp from "../../util/timestamp";
import {type NodeEntry} from "./nodeEntry";
import {
type EdgeSpec,
@ -14,7 +13,7 @@ import {
const exampleEntry = (overrides: $Shape<NodeEntry>): NodeEntry => ({
key: "sample-title",
title: "Sample title",
timestampMs: Timestamp.fromNumber(123),
timestampMs: 123,
contributors: [],
weight: 456,
...overrides,
@ -23,7 +22,7 @@ const exampleEntry = (overrides: $Shape<NodeEntry>): NodeEntry => ({
describe("plugins/initiatives/edgeSpec", () => {
describe("normalizeEdgeSpec", () => {
it("should add empty arrays when missing", () => {
const timestampMs: TimestampMs = Timestamp.fromNumber(321);
const timestampMs: TimestampMs = 321;
const json: EdgeSpecJson = {};
const actual = normalizeEdgeSpec(json, timestampMs);
@ -31,7 +30,7 @@ describe("plugins/initiatives/edgeSpec", () => {
});
it("should preserve urls when present", () => {
const timestampMs: TimestampMs = Timestamp.fromNumber(321);
const timestampMs: TimestampMs = 321;
const json: EdgeSpecJson = {
urls: ["https://foo.bar/a", "https://foo.bar/b"],
};
@ -41,7 +40,7 @@ describe("plugins/initiatives/edgeSpec", () => {
});
it("should normalize any entries from json format", () => {
const timestampMs: TimestampMs = Timestamp.fromNumber(321);
const timestampMs: TimestampMs = 321;
const json: EdgeSpecJson = {
entries: [{title: "Minimal example"}],
};
@ -62,7 +61,7 @@ describe("plugins/initiatives/edgeSpec", () => {
});
it("should throw on entries with the same key", () => {
const timestampMs: TimestampMs = Timestamp.fromNumber(321);
const timestampMs: TimestampMs = 321;
const json: EdgeSpecJson = {
entries: [
{key: "1", title: "first"},

16
src/plugins/initiatives/nodeEntry.test.js
View File

@ -52,14 +52,14 @@ describe("plugins/initiatives/nodeEntry", () => {
describe("normalizeNodeEntry", () => {
it("should throw without a title", () => {
const timestampMs: TimestampMs = Timestamp.fromNumber(123);
const timestampMs: TimestampMs = 123;
const entry: NodeEntryJson = {key: "no-title"};
const f = () => normalizeNodeEntry(entry, timestampMs);
expect(f).toThrow(TypeError);
});
it("should handle a minimal entry", () => {
const timestampMs: TimestampMs = Timestamp.fromNumber(123);
const timestampMs = 123;
const entry: NodeEntryJson = {title: "Most minimal"};
const expected: NodeEntry = {
title: "Most minimal",
@ -72,7 +72,7 @@ describe("plugins/initiatives/nodeEntry", () => {
});
it("should handle an entry with weights", () => {
const timestampMs: TimestampMs = Timestamp.fromNumber(123);
const timestampMs: TimestampMs = 123;
const entry: NodeEntryJson = {title: "Include weight", weight: 42};
const expected: NodeEntry = {
title: "Include weight",
@ -85,7 +85,7 @@ describe("plugins/initiatives/nodeEntry", () => {
});
it("should handle an entry with contributors", () => {
const timestampMs: TimestampMs = Timestamp.fromNumber(123);
const timestampMs: TimestampMs = 123;
const entry: NodeEntryJson = {
title: "Include contributors",
contributors: ["https://foo.bar/u/abc"],
@ -101,7 +101,7 @@ describe("plugins/initiatives/nodeEntry", () => {
});
it("should handle an entry with timestamp", () => {
const timestampMs: TimestampMs = Timestamp.fromNumber(123);
const timestampMs: TimestampMs = 123;
const entry: NodeEntryJson = {
title: "Include timestamp",
timestampIso: Timestamp.toISO(Date.parse("2018-02-03T12:34:56.789Z")),
@ -110,16 +110,14 @@ describe("plugins/initiatives/nodeEntry", () => {
title: "Include timestamp",
key: "include-timestamp",
contributors: [],
timestampMs: Timestamp.fromNumber(
Date.parse("2018-02-03T12:34:56.789Z")
),
timestampMs: Date.parse("2018-02-03T12:34:56.789Z"),
weight: null,
};
expect(normalizeNodeEntry(entry, timestampMs)).toEqual(expected);
});
it("should handle an entry with key", () => {
const timestampMs: TimestampMs = Timestamp.fromNumber(123);
const timestampMs: TimestampMs = 123;
const entry: NodeEntryJson = {
title: "Include key",
key: "much-different-key",

13
src/util/timestamp.js
View File

@ -9,7 +9,7 @@
*/
// A timestamp representation in ms since epoch.
export opaque type TimestampMs: number = number;
export type TimestampMs = number;
// A timestamp representation in ISO 8601 format.
export opaque type TimestampISO: string = string;
@ -22,7 +22,7 @@ export opaque type TimestampISO: string = string;
* than a forced refactor across the codebase.
*/
export function toISO(timestampLike: TimestampMs | number): TimestampISO {
const timestampMs: TimestampMs = fromNumber(timestampLike);
const timestampMs: TimestampMs = validate(timestampLike);
return new Date(timestampMs).toISOString();
}
@ -47,13 +47,12 @@ export function fromISO(timestampISO: TimestampISO): TimestampMs {
}
/**
* Creates a TimestampMs from a number input.
* Validate that a number is potentially a valid timestamp.
*
* Since much of the previous types have used `number` as a type instead of
* TimestampMs. Accepting `number` will give an easier upgrade path, rather
* than a forced refactor across the codebase.
* This checks that the number is a finite integer, which avoids some potential
* numbers that are not valid timestamps.
*/
export function fromNumber(timestampMs: number): TimestampMs {
export function validate(timestampMs: number): TimestampMs {
const asNumber = Number(timestampMs);
if (
timestampMs === null ||

28
src/util/timestamp.test.js
View File

@ -1,7 +1,7 @@
// @flow
import type {TimestampMs, TimestampISO} from "./timestamp";
import {fromISO, toISO, fromNumber} from "./timestamp";
import {fromISO, toISO, validate} from "./timestamp";
/**
* Helper function to write readable "toThrow" tests. This intentionally
@ -33,14 +33,14 @@ describe("util/timestamp", () => {
describe("roundtrips", () => {
it("should handle number roundtrips", () => {
const origin: number = fullExample.ms;
expect(fromNumber(origin)).toBe(origin);
expect(validate(origin)).toBe(origin);
expect(fromISO(toISO(origin))).toBe(origin);
expect(fromNumber(fromISO(toISO(origin)))).toBe(origin);
expect(validate(fromISO(toISO(origin)))).toBe(origin);
});
it("should handle ISO roundtrips", () => {
const origin: TimestampISO = fullExample.ISO;
expect(toISO(fromISO(origin))).toBe(origin);
expect(toISO(fromNumber(fromISO(origin)))).toBe(origin);
expect(toISO(validate(fromISO(origin)))).toBe(origin);
});
});
@ -96,34 +96,34 @@ describe("util/timestamp", () => {
});
});
describe("fromNumber", () => {
describe("validate", () => {
it("should throw on null", () => {
given(null).expect(fromNumber).toThrow(TypeError);
given(null).expect(validate).toThrow(TypeError);
});
it("should throw on undefined", () => {
given(undefined).expect(fromNumber).toThrow(TypeError);
given(undefined).expect(validate).toThrow(TypeError);
});
it("should throw on NaN", () => {
given(NaN).expect(fromNumber).toThrow(TypeError);
given(NaN).expect(validate).toThrow(TypeError);
});
it("should throw on Infinity", () => {
given(Infinity).expect(fromNumber).toThrow(TypeError);
given(Infinity).expect(validate).toThrow(TypeError);
});
it("should throw on floating point numbers", () => {
given(1 / 3)
.expect(fromNumber)
.expect(validate)
.toThrow(TypeError);
});
it("should handle 0 correctly", () => {
expect(fromNumber(0)).toBe(0);
expect(validate(0)).toBe(0);
});
it("should handle Date.now correctly", () => {
const now = Date.now();
expect(fromNumber(now)).toBe(now);
expect(validate(now)).toBe(now);
});
it("should handle examples correctly", () => {
expect(fromNumber(fullExample.ms)).toBe(fullExample.ms);
expect(fromNumber(partialExample.ms)).toBe(partialExample.ms);
expect(validate(fullExample.ms)).toBe(fullExample.ms);
expect(validate(partialExample.ms)).toBe(partialExample.ms);
});
});
});