Skip to content
This repository has been archived by the owner. It is now read-only.

/ browser-laptop Archived

Synopsis test verify instrumentation #4189

Closed
wants to merge 5 commits into from
Closed

Synopsis test verify instrumentation #4189

wants to merge 5 commits into from

Conversation

@mrose17
Copy link
Member

mrose17 commented Sep 22, 2016

Auditor: @ayumi

Fixes #4122

Test Plan:

  1. mkdir ~/.brave-test-ledger
  2. NODE_ENV=test npm start
  3. open the payments panel, visit any site, go to another site after less than 8s
  4. verify that about:preferences#payments shows the traffic, e.g.,
    1
mrose17 added 3 commits Sep 22, 2016
@mrose17
when the idle state changes, flush the timer
Loading status checks…
ffcac85 
fixes #4051
@mrose17
do not use ~/.brave-test-ledger for ledger files when NODE_ENV=test
Loading status checks…
8023cae 
cf., #4122
@mrose17
initialize synopsis options min* regardless of whether ledger-synopsi…
Loading status checks…
6a9a1da 
…s.json already exists

if the synopsis is created (rather than restored), the `options.min*`
properties were not initialized.
@mrose17 mrose17 added this to the 0.12.2dev milestone Sep 22, 2016
@mrose17 mrose17 self-assigned this Sep 22, 2016
@diracdeltas
diracdeltas reviewed Sep 22, 2016
View changes
app/ledger.js

return path.join(basePath, parts.name + parts.ext)
return path.join(app.getPath('userData'), parts.name + parts.ext)

This comment has been minimized.

Sign in to view
Copy link
@diracdeltas

diracdeltas Sep 22, 2016

Member

please revert this change. tests can't use the userData directory. https://travis-ci.org/brave/browser-laptop/builds/161965938

This comment has been minimized.

Sign in to view
Copy link
@mrose17

mrose17 Sep 22, 2016

Author Member

fine.

This comment has been minimized.

Sign in to view
Copy link
@diracdeltas

diracdeltas Sep 22, 2016

Member

this is probably ok as of 3c1e03c now that i look at it, but there is some weirdness going on with tests right now

This comment has been minimized.

Sign in to view
Copy link
@diracdeltas

diracdeltas Sep 22, 2016

Member

i take that back, i think this is fine. test failure was intermittent.
@mrose17
Revert "do not use ~/.brave-test-ledger for ledger files when NODE_EN…
Loading status checks…
35380ae 
…V=test"

This reverts commit 8023cae.
@mrose17
Copy link
Member Author

mrose17 commented Sep 22, 2016

thanks. i wouldn't be surprised about their being some weirdness. (-;

it's not clear to me how to test stuff like this in an automated fashion.

@ayumi - your thoughts?
@ayumi
Copy link
Contributor

ayumi commented Sep 22, 2016 •
edited

Looks like when the idle state changes, flush the timer ffcac85 from #4186 is also in this PR
@mrose17
Copy link
Member Author

mrose17 commented Sep 22, 2016

one is built on top of the other, so we have to review, approve them in that order, i think.
@mrose17 mrose17 modified the milestones: 0.12.3dev, 0.12.2dev Sep 22, 2016
@ayumi
Copy link
Contributor

ayumi commented Sep 22, 2016

Branch base– cool– it would be helpful to note the base in the PR

I tried this PR and it works; lgtm
@mrose17
Copy link
Member Author

mrose17 commented Sep 22, 2016

thanks. let me get the other PR resolved then we can merge this one.
@mrose17
Revert "Revert "do not use ~/.brave-test-ledger for ledger files when…
Loading status checks…
22942dd 
… NODE_ENV=test""

This reverts commit 35380ae.
mrose17 added a commit that referenced this pull request Sep 22, 2016
@mrose17
supersede synopsis-related PRs to get a clean merge
Loading status checks…
d22778d 
replaces #4189 and
#4186
@mrose17 mrose17 mentioned this pull request Sep 22, 2016
Merged
@mrose17
Copy link
Member Author

mrose17 commented Sep 22, 2016

superseded by #4194
@mrose17 mrose17 closed this Sep 22, 2016
@mrose17 mrose17 deleted the synopsis-test-verify-instrumentation branch Sep 22, 2016
@bbondy bbondy modified the milestones: 0.12.2dev, 0.12.3dev Sep 23, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@diracdeltas diracdeltas

Assignees

@mrose17 mrose17

Labels
feature/rewards
Projects
None yet
Milestone
  0.12.2dev
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@mrose17 @ayumi @diracdeltas @bbondy
You can’t perform that action at this time.