Skip to content

Conversation

fboucquez
Copy link
Contributor

@fboucquez fboucquez commented Dec 12, 2019

Hi @rg911

This PRs adds the RepositoryFactory and utilizes the interfaces a bit more than the concrete classes working better with abstractions.

Code improvements:

These are the improvements added to the code base. They are not breaking changes for the user (the only exception is TransactionService constructor, see below).

  1. RepositoryFactory that creates repositories. The basic implementation is the RepositoryFactoryHttp that creates the Http clients. The user should create one RepositoryFactory per application/network and reuse that object in the different components when repository/http objects are required.
  2. Repository Factory knows how to load and cache NetworkType and Generation Hash. It also allows users to provide their own Network Type (as requested in Add networkType to http clients constructors #361). Network type and generation hash are only loaded once for the lifetime of the factory reducing I/O and the chances of being banned.
  3. Services are using repository interfaces rather than HTTP concrete classes. The only breaking change is the TransactionService constructor, it was creating the HTTP objects itself missing some arguments like the network type required in 361.
  4. Created IListener interface (I prefer them to be Listener interface and ListenerHttp but I didn't want to rename the current Listener class)
  5. Fixed some typos.

Test improvements:

This PR also includes fairly big e2e improvements.

  1. IntegrationTestHelper is a helper object that allows e2e tests to be configured and executed. It contains some helper functions that reduce the size of a test.
  2. The helper knows who to load the generated nemesis YAML file from catapult-service-bootstrap You don't need to update your configuration every time you reset
  3. The generation hash and network type are not hard coded anymore in the test or in the config files. You can run against different networks. This is good timing as bootstrap has been moved to a public configuration. You don't need to manually copy the generation hash from http://localhost:3000/block/1 every time there is a bootstrap reset.
  4. MIJIN_TEST hardcoded values have been removed from e2e. You can now basically run e2e tests against different networks without code change. The network type is resolved on boot when loading the network type using rest.
  5. It's a step forward automatic integration test. Jenkins/Travis could run a brand new catapult-service-bootstrap and run the e2e tests without user intervention (every build/every day). There is no need to copy and paste accounts and generation hash.
  6. There is no need to start a new listener for every test. The IntegrationTestHelper holds the e2e test file's listener. This reduces the size of a test and it's a good test for scalability. In some scenarios, it would be great to have one listener running for longer that one operation (maybe for the whole life of the application). You can still create extra listeners if you like per test.
  7. MaxFee is added to all the e2e transactions so it works with the default bootstrap configuration. If not, a transaction will just timeout.
  8. Removed unnecessary sleep/timeout. Endpoints are up to date a soon as a transaction is completed.
  9. Fixed deadline test to work when the computer's timezone is not UTC.
  10. Moved CreateTransactionFromDTO.spec.ts from e2e to tests
  11. E2E tests started using promises instead of done() callbacks.

All HTTP and Service e2e tests have been migrated to IntegrationTestHelper. Overall, the size of a e2e tests have been reducing by 30-40 % without losing configurability (you can still create the transaction the way you want it) and adding all the features listed above.

I think this PR is ready to be reviewed, fixed if necessary and approved.

Fixed deadline test to work when your timezone is not UTC
Updated services to use repositories rather than http objects.
@fboucquez fboucquez requested a review from rg911 December 12, 2019 20:13
@rg911 rg911 merged commit b26694b into symbol:master Dec 22, 2019
@fboucquez fboucquez deleted the task/g381_repositoryInterfacesAndFactory branch December 22, 2019 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants