Reenable test bigchain api - part 1 by z-bowen · Pull Request #2415 · bigchaindb/bigchaindb

z-bowen · 2018-07-27T14:12:23Z

Problem: Tests in class TestBigChainAPI from tests/db/test_bigchain_api.py were disabled

Solution

Fixed the tests that still made sense, and deleted the rest.

Solution: Remove core.py. Refactor BigchainDB Class to remove inheritance from Bigchain.

…its, as I don't know what I'm doing, and I can't experiment without running the CI... Sorry in advance!

…gchaindb into z-bowen-remove_bigchain_class

…ivate method, so I had to align with that.

Solution: resolved merge conflicts to excavate Block class from my branch

Solution: Merged and repushed

Solution: Merge changes into Master

…n_api.py` were disabled Solution: Fixed the tests that still made sense, and deleted the rest.

kansi · 2018-07-27T14:15:35Z

tests/conftest.py

    from bigchaindb.models import Transaction
    # create blocks with transactions for `USER` to spend
-    for block in range(4):
+    print('Fuck you')


I think we should remove this.

kansi · 2018-07-27T14:16:03Z

tests/db/test_bigchain_api.py

-
 class TestBigchainApi(object):

    @pytest.mark.genesis


The genesis marker should not be needed anymore

Solution: Made things prettier

codecov-io · 2018-07-27T14:19:00Z

Codecov Report

Merging #2415 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #2415      +/-   ##
=========================================
+ Coverage   88.16%   88.2%   +0.03%     
=========================================
  Files          40      40              
  Lines        2298    2297       -1     
=========================================
  Hits         2026    2026              
+ Misses        272     271       -1

Solution: Deleted the debugging comments

Solution: Removed the decorator

ldmberman · 2018-07-27T14:25:56Z

tests/conftest.py

    from bigchaindb.models import Transaction
    # create blocks with transactions for `USER` to spend
-    for block in range(4):
+    for height in range(1, 4):


Why not range(0, 3)?

height 0 is assumed for genesis block although changing to range(0,3) shouldn't matter either

ldmberman · 2018-07-27T14:30:33Z

tests/db/test_bigchain_api.py

-        vote = b.vote(block3.id, b.get_last_voted_block().id, True)
-        b.write_vote(vote)
-
-        with pytest.raises(CriticalDoubleInclusion):


Now when you updated these tests you can remove CriticalDoubleInclusion from bigchaindb.exceptions.

Cool! Done.

ldmberman · 2018-07-27T14:34:38Z

tests/db/test_bigchain_api.py

        transfer_tx2 = transfer_tx2.sign([alice.private_key])
-        block3 = b.create_block([transfer_tx2])
-        b.write_block(block3)
+        with pytest.raises(OperationError):


Judging by the name and the previous code of the test, we need to test that BigchainDB.get_spent raisesCriticalDoubleSpend here.

No, this isn't a test for a double spend. It's testing that you can't have two txs with the same tx_id

It's called test_get_spent_with_double_inclusion_detected and it was calling b.get_spent(tx.id, 0) expecting it to raise CriticalDoubleInclusion, which is like double spend that somehow sneaked into the database. Currently, BigchainDB.get_spent raises core_exceptions.CriticalDoubleSpend and DoubleSpend, which is what we by analogy need to test now.

ldmberman · 2018-07-27T14:38:40Z

tests/db/test_bigchain_api.py

-        vote = b.vote(block3.id, b.get_last_voted_block().id, True)
-        b.write_vote(vote)
+        with pytest.raises(DoubleSpend):
+            b.validate_transaction(transfer_tx2, [transfer_tx])


Also judging by the previous state, we should test how BigchainDB.get_spent raises DoubleSpend when called with current_transactions=[transfer_tx].

This is actually the method that ends up throwing the error in this test. See bigchaindb/models.py:59-63.

This is a bit over complicated for a unit test (we should probably be invoking that method directly,) but it is covered, so I don't think we need an additional test for it.

What works for me is keeping everything as it is. I change all the block creation to a store_bulk_transaction and in the end when I call

with pytest.raises(CriticalDoubleSpend): b.get_spent(tx.id, 0)

the CriticalDoubleSpend is raised and the test passes.

I think later on in the file we test an actual DoubleSpend. With this it's doubled.

The duplicate coverage is in the second test class, which hasn't been cleaned up yet, so I'm not removing my original asserts, but I added the assertion you requested

But then this one will be doubled?

ldmberman · 2018-07-27T14:39:47Z

tests/db/test_bigchain_api.py

-        # vote the first block invalid
-        vote = b.vote(block1.id, b.get_last_voted_block().id, False)
-        b.write_vote(vote)
+        with pytest.raises(OperationError):


Does this test differ somehow from the one before the one above?

Other than that the first test is for a transfer and this is for a new asset, no. I don't know if that's a significant enough difference to justify a second test, but it's already written, so...

It seems that this test tries to brute force storing a duplicate transaction and the above one tries to check if the double spend transaction in fact raises an exception when validate_transaction is called.

ldmberman · 2018-07-27T14:41:59Z

tests/db/test_bigchain_api.py

-    def test_create_genesis_block_fails_if_table_not_empty(self, b):
-        from bigchaindb.common.exceptions import GenesisBlockAlreadyExistsError
-
-        with pytest.raises(GenesisBlockAlreadyExistsError):


This exception can be removed from bigchaindb/common/exceptions.py now.

…e in the code Solution: Deleted it

ldmberman · 2018-07-27T14:47:26Z

tests/db/test_bigchain_api.py

+            tx.validate(b)


 class TestTransactionValidation(object):


There are tests still disabled starting from this one. Do you plan to enable them in a separate PR?

Yes. I'll keep working on this until I've done all the tests in this file, but this seemed like a natural break point where I could commit some functionality.

Yes, I think we should merge this one and make a part two PR instead. The file has just too many changes in the end

Solution: Deleted it

codegeschrei · 2018-08-09T16:05:31Z

tests/db/test_bigchain_api.py

@@ -216,12 +88,7 @@ def test_text_search(self, b, alice):
                                 asset=asset3).sign([alice.private_key])

        # create the block


this needs to be removed or changed

Are you referring to the comment about creating the block, or something in the code?

the comment, we don't create a block here anymore

codegeschrei · 2018-08-09T16:22:34Z

tests/db/test_bigchain_api.py

-        }
-
-    @pytest.mark.usefixtures('inputs')
-    def test_write_transaction(self, b, user_pk, user_sk):


why did you remove this one?

codegeschrei · 2018-08-09T16:23:05Z

tests/db/test_bigchain_api.py

-        assert tx_from_db.to_dict() == tx.to_dict()
-
-    @pytest.mark.usefixtures('inputs')
-    def test_read_transaction(self, b, user_pk, user_sk):


same here, why is it removed?

I replaced the first test. This one seems very similar to the first... Basically, we write something to the DB, get it back, and check that nothing has changed. I don't see a way to keep these tests distinct with BDB2.0

… that everything is stored faithfully Solution: Replaced it and got it working again

z-bowen added 20 commits June 28, 2018 12:01

Problem: core.py contains an unused class, Bigchain

920fe2b

Solution: Remove core.py. Refactor BigchainDB Class to remove inheritance from Bigchain.

Problem: core.py contains an unused class, Bigchain

e71fd5d

Solution: Remove core.py. Refactor BigchainDB Class to remove inheritance from Bigchain.

Merge branch 'z-bowen-remove_bigchain_class'

97946bd

Merge remote-tracking branch 'upstream/master'

d5ab427

Fixed flake8 complaint about too many blank lines

a3b9bb6

Attempting to fix Sphinx docs. This may result in some redundant comm…

80c90c7

…its, as I don't know what I'm doing, and I can't experiment without running the CI... Sorry in advance!

Attempting to fix Sphinx docs. This may result in some redundant comm…

8176fc6

…its, as I don't know what I'm doing, and I can't experiment without running the CI... Sorry in advance!

Merge branch 'remove_bigchain_class' of https://github.com/z-bowen/bi…

47b08ea

…gchaindb into z-bowen-remove_bigchain_class

Merge branch 'z-bowen-remove_bigchain_class'

1b2e9b8

Merge remote-tracking branch 'upstream/master'

78d7902

Updating from master changed BigchainDB.process_post_response to a pr…

f42c6ad

…ivate method, so I had to align with that.

Fixed a couple stale references to bigchaindb.Bigchain in docstrings

8f18948

Missed a reference to Bigchain in a patch call...

5dbe4e4

*Problem: Merge conflict with removal of Block class

f33924a

Solution: resolved merge conflicts to excavate Block class from my branch

*Problem: Merge conflicts with bigchaindb#2342

93c011e

Solution: Merged and repushed

*Problem: Merge conflict from bigchaindb#2342

c6b1011

Solution: Merge changes into Master

Merge branch 'master' of github.com:bigchaindb/bigchaindb

a055583

Merge branch 'master' of github.com:bigchaindb/bigchaindb

2dfe46a

Merge branch 'master' of github.com:bigchaindb/bigchaindb

0a65cb5

Problem: Tests in class TestBigChainAPI from `tests/db/test_bigchai…

a231c06

…n_api.py` were disabled Solution: Fixed the tests that still made sense, and deleted the rest.

z-bowen requested review from kansi, ldmberman and ttmc July 27, 2018 14:12

kansi reviewed Jul 27, 2018

View reviewed changes

Problem: Flake8 style complaints

df3078b

Solution: Made things prettier

z-bowen added 2 commits July 27, 2018 16:20

Problem: Debugging comments were slightly NSFW

99c7c6a

Solution: Deleted the debugging comments

Problem: Stale use of pytest.mark.genesis decorator on these tests

21688b7

Solution: Removed the decorator

ldmberman reviewed Jul 27, 2018

View reviewed changes

Problem: CriticalDoubleInclusion exception was no longer used anywher…

2a1851a

…e in the code Solution: Deleted it

ldmberman reviewed Jul 27, 2018

View reviewed changes

Problem: Unused exception GenesisBlockAlreadyExistsError

5bc681d

Solution: Deleted it

kansi mentioned this pull request Jul 29, 2018

Problem: Tests cleanup incomplete #2381

Closed

16 tasks

ttmc removed their request for review July 29, 2018 13:12

ttmc changed the title ~~Reenable test bigchain api~~ Reenable test bigchain api - part 1 Jul 29, 2018

riyazlafir mentioned this pull request Jul 30, 2018

Bigchaindb network validator #2418

Closed

Merge branch 'master' of github.com:bigchaindb/bigchaindb

2799e50

kansi approved these changes Aug 7, 2018

View reviewed changes

z-bowen added 3 commits August 8, 2018 10:48

Merge branch 'master' of github.com:bigchaindb/bigchaindb

2067115

Merge branch 'master' of github.com:bigchaindb/bigchaindb

e18632b

Merge branch 'master' into reenable-test_bigchain_api

dacdf58

codegeschrei reviewed Aug 9, 2018

View reviewed changes

Problem: I removed a test that covered writing to the DB and checking…

2167af8

… that everything is stored faithfully Solution: Replaced it and got it working again

codegeschrei approved these changes Aug 13, 2018

View reviewed changes

codegeschrei merged commit 3092beb into bigchaindb:master Aug 13, 2018

codegeschrei mentioned this pull request Aug 14, 2018

Problem: tests in test_bigchain_api are not activated #2456

Merged

		@@ -216,12 +88,7 @@ def test_text_search(self, b, alice):
		asset=asset3).sign([alice.private_key])

		# create the block

Conversation

z-bowen commented Jul 27, 2018

Solution

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kansi Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codegeschrei Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Jul 27, 2018 •

edited

Loading

kansi Jul 27, 2018 •

edited

Loading

codegeschrei Aug 9, 2018 •

edited

Loading