Skip to content
This repository was archived by the owner on Nov 15, 2021. It is now read-only.

Fix a couple of tests #652

Merged
merged 4 commits into from
Oct 8, 2018
Merged

Fix a couple of tests #652

merged 4 commits into from
Oct 8, 2018

Conversation

ixje
Copy link
Member

@ixje ixje commented Oct 7, 2018

What current issue(s) does this address, or what feature is it adding?
There are a couple of tests that didn't work as intended under certain conditions, had uncaught exceptions or otherwise. This cleans up a couple of them

  1. This test was passing but internally had an exception that shows up in logs

    def test_6_weird_amount(self):

  2. This test would cause a 404 error on downloading, while actually expected it kept triggering me to look into why.
    https://github.com/CityOfZion/neo-python/blob/master/neo/Prompt/Commands/tests/test_bootstrap.py#L24

  3. This test would fail if your configured theme is light instead of the default dark

    def test_prefs_json_empty(self):

  4. one test used the deprecated assertEquals instead of assertEqual which kept throwing a warning

  5. the BlockchainFixtureTestCase and WalletFixtureTestCase relied on running the tests from the root folder. My IDE (PyCharm) executes from whatever file I press run test and for tests using either of the before mentioned cases it would fail to find wallets due to the relative path construction e.g.

    def wallet_1_path(cls):
    return './fixtures/neo-test1-w.wallet'

How did you solve this problem?

  1. catch exception
  2. don't assume default theme, but actually read what theme is configured
  3. added a clear notification indicating the test shows an error
  4. use assertEqual
  5. detect root folder and create absolute paths.

How did you make sure your solution works?
make test

Are there any special changes in the code that we should be aware of?
no
Please check the following, if applicable:

  • Did you add any tests?
  • Did you run make lint?
  • Did you run make test?
  • Are you making a PR to a feature branch or development rather than master?
  • [] Did you add an entry to CHANGELOG.rst? (if not, please do)

@coveralls
Copy link

coveralls commented Oct 7, 2018

Coverage Status

Coverage decreased (-0.1%) to 81.278% when pulling 390fd94 on ixje:fix_tests into 69bead6 on CityOfZion:development.

ixje added 2 commits October 7, 2018 17:12
- Add clear notification message that the test produces a 404 error
@ixje ixje changed the title Quick fix 2 test Fix a couple of tests Oct 7, 2018
@jseagrave21
Copy link
Contributor

Good fixes. I also caught the Prompt.Commands.Send fix. It is included in #610

@ixje ixje changed the base branch from master to development October 8, 2018 18:03
@ixje ixje merged commit dcffbd6 into CityOfZion:development Oct 8, 2018
@ixje ixje deleted the fix_tests branch October 8, 2018 18:08
@jseagrave21
Copy link
Contributor

@ixje Hey, should there have been an update to the changelog?

@ixje
Copy link
Member Author

ixje commented Oct 8, 2018

I initially chose not to do it because it was 2 minor fixes, then extended it with 2 more. I'm ok without a changelog on these. Most will not notice it as they've been there for such a long time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants