Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

basic tests for subscribeToEvents #4115

Merged
merged 2 commits into from
Jan 11, 2017
Merged

Conversation

derhuerst
Copy link
Contributor

Follow-up of #4066, which changed subscribeToEvent to subscribeToEvents without any tests checking for regressions.

@derhuerst derhuerst added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 10, 2017
const { api, contract } = this;

subscribeToEvents(contract, [ 'Foo', 'Bar' ]);
await delay(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Love the use of await, really do :)

However, wouldn't it make more sense when using timers to rather use the sinon stubs? (eg. sinon.useFakeTimers())

Rest looks fine.

That way you can step through and se the clocks to whichever times you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about that as well. Decided for await delay(9) (which is somewhat random tbh) because using fake timers introduces potential errors & bugs by manipulating the global timing functions.

Have no real opinion on this, will adapt to whatever you and @ngotchac prefer.

Copy link
Contributor

@jacogr jacogr Jan 10, 2017

Choose a reason for hiding this comment

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

Not going to grumble, just think that we have a bit more control with actually moving the timer ahead. Well, I've had some success with it.

https://github.com/ethcore/parity/blob/7cdfaf1a439a5287b55b4195b29878193f0d7049/js/src/3rdparty/shapeshift/shapeshift.spec.js#L173

Either way, what we have works.

(Only error, as with any stubbing, is not restoring - beforeEach/afterEach to the rescue)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to use sinon sandboxes (they are really great) but they don't have reset, only restore. That's why I chose functions to create the mocks.

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 11, 2017
@gavofyork gavofyork merged commit 4ecd9da into master Jan 11, 2017
@gavofyork gavofyork deleted the jr-subscribe-to-events-tests branch January 11, 2017 11:14
arkpar pushed a commit that referenced this pull request Jan 12, 2017
* subscribeToEvent fixtures ✅

* subscribeToEvent tests ✅
arkpar added a commit that referenced this pull request Jan 12, 2017
* Fix broken transfer total balance (#4127)

* Add proper label to method decoding inputs (#4136)

* Another minor estimation fix (#4133)

* Return 0 instead of error with out of gas on estimate_gas

* Fix stuff up.

* Another estimate gas fix.

* Alter balance to maximum possible rather than GP=0.

* Only increase to amount strictly necessary.

* Get rid of unsafe code in ethkey, propagate incorrect Secret errors. (#4119)

* Implementing secret

* Fixing tests

* Refactor VoteCollector (#4101)

* dir

* simple validator list

* stub validator contract

* make the engine hold Weak<Client> instead of IoChannel

* validator set factory

* register weak client with ValidatorContract

* check chain security

* add address array to generator

* register provider contract

* update validator set on notify

* add validator contract spec

* simple list test

* split update and contract test

* contract change

* use client in tendermint

* fix deadlock

* step duration in params

* adapt tendermint tests

* add storage fields to test spec

* constructor spec

* execute under wrong address

* create under correct address

* revert

* validator contract constructor

* move genesis block lookup

* add removal ability to contract

* validator contract adding validators

* fix basic authority

* validator changing test

* more docs

* update sync tests

* remove env_logger

* another env_logger

* cameltoe

* hold EngineClient instead of Client

* return error on misbehaviour

* nicer return

* sprinkle docs

* Reenable mainnet update server. (#4137)

* basic tests for subscribeToEvents (#4115)

* subscribeToEvent fixtures ✅

* subscribeToEvent tests ✅

* temporarily skip failing test (#4138)

* Improvements and optimisations to estimate_gas (#4142)

* Return 0 instead of error with out of gas on estimate_gas

* Fix stuff up.

* Another estimate gas fix.

* Alter balance to maximum possible rather than GP=0.

* Only increase to amount strictly necessary.

* Improvements and optimisations to estimate_gas.

- Introduce proper error type
- Avoid building costly traces

* Fix tests.

* Actually fix testsActually fix tests

* Use estimateGas error (as per updated implementation) (#4131)

* Use estimateGas error (as per updated implementation)

* EXCEPTION_ERROR as per #4142

* Better error log reporting & handling (#4128)

* Don't pop-up notifications after network switch (#4076)

* Better notifications

* Don't pollute with notifs if switched networks

* Better connection close/open events / No more notifs on change network

* PR Grumbles

* Add close and open events to HTTP // Add tests

* Fix tests

* WIP Signer Fix

* Fix Signer // Better reconnection handling

* PR Grumbles

* PR Grumbles

* Fixes wrong fetching of balances + Notifications

* Secure API WIP

* Updated Secure API Connection + Status

* Linting

* Linting

* Updated Secure API Logic

* Proper handling of token updates // Fixing poping notifications

* PR Grumbles

* PR Grumbles

* Fixing tests

* Trim spaces from InputAddress (#4126)

* Trim spaces for addresses

* onSubmit has only value, not event

* onSubmit (again)

* Length check on trimmed value

* Remove bindActionCreators({}, dispatch) (empty) (#4135)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants