Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: do not use built-in random and add proper linter check #698

Merged
merged 1 commit into from
Jul 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/docker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@ jobs:
python extras/github/docker.py
- name: Check version
if: steps.prep.outputs.check-version
run: |
make check-version VERSION='${{ steps.prep.outputs.check-version }}'
env:
VERSION: ${{ steps.prep.outputs.check-version }}
run: make check-custom
- name: Set up QEMU # arm64 is not available natively
uses: docker/setup-qemu-action@v2
- name: Set up Docker Buildx
Expand Down
10 changes: 5 additions & 5 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,15 @@ isort-check:
yamllint:
yamllint .

.PHONY: check-version
check-version:
bash ./extras/check_version.sh $(VERSION)
Copy link
Collaborator

@luislhl luislhl Jul 12, 2023

Choose a reason for hiding this comment

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

I think we will need to adapt the .github/workflows/docker.yml, because we used this arg there.

Since you ported it to be an env var, we should set it as an env var there instead. Besides migrating to the new check-custom there as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right, I'll fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I've fixed it and tested with act with a wrong and correct tag and both work as expected.

.PHONY: check-custom
check-custom:
bash ./extras/custom_checks.sh

.PHONY: check
check: check-version yamllint flake8 isort-check mypy
check: check-custom yamllint flake8 isort-check mypy

.PHONY: dcheck
dcheck: check-version yamllint flake8 isort-check dmypy
dcheck: check-custom yamllint flake8 isort-check dmypy

# formatting:

Expand Down
47 changes: 0 additions & 47 deletions extras/check_version.sh

This file was deleted.

102 changes: 102 additions & 0 deletions extras/custom_checks.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
#!/bin/bash

# Define colors
RED='\033[0;31m'
GREEN='\033[0;32m'
NC='\033[0m' # No Color

# Source dirs
SOURCE_DIRS=(hathor tests)

# Define your custom linter check functions here
# Each function should return 0 if everything is OK, and 1 if something is wrong.

function check_version_match() {
# This function will check all source files containing the project version and return 1 in case
# they don't match. When a version is provided as an environment variable, it is checked against the package version.

OPENAPI_FILE="hathor/cli/openapi_files/openapi_base.json"
SRC_FILE="hathor/version.py"
PACKAGE_FILE="pyproject.toml"

OPENAPI_VERSION=`grep "version\":" ${OPENAPI_FILE} | cut -d'"' -f4`
SRC_VERSION=`grep "BASE_VERSION =" ${SRC_FILE} | cut -d "'" -f2`
PACKAGE_VERSION=`grep '^version' ${PACKAGE_FILE} | cut -d '"' -f2`

# For debugging:
# echo x${SRC_VERSION}x
# echo x${OPENAPI_VERSION}x
# echo x${PACKAGE_VERSION}x

EXITCODE=0

if [[ x${PACKAGE_VERSION}x != x${SRC_VERSION}x ]]; then
echo "Version different in ${PACKAGE_FILE} and ${SRC_FILE}"
EXITCODE=1
fi

if [[ x${PACKAGE_VERSION}x != x${OPENAPI_VERSION}x ]]; then
echo "Version different in ${PACKAGE_FILE} and ${OPENAPI_FILE}"
EXITCODE=1
fi

# We expect an optional environment variable containing a version string to be checked against the others
if [[ -n ${VERSION} ]]; then
if [[ x${PACKAGE_VERSION}x != x${VERSION}x ]]; then
echo "Version different in ${PACKAGE_FILE} and VERSION environment variable"
EXITCODE=1
fi
fi

return $EXITCODE
}

function check_do_not_use_builtin_random_in_tests() {
# If the check fails, return 1
# If the check passes, return 0
exclude=(
hathor/merged_mining/debug_api.py
hathor/client.py
hathor/cli/tx_generator.py
)
exclude_params=()
for item in "${exclude[@]}"; do
exclude_params+=(-not -path "*$item*")
done
if find "${SOURCE_DIRS[@]}" "${exclude_params[@]}" -type f -print0 | xargs -0 grep -l '\<import .*\<random\>'; then
echo '"import random" found in the files above'
echo 'use `self.rng` or `hathor.util.Random` instead of `random`'
return 1
fi
return 0
}

# List of functions to be executed
checks=(
check_version_match
check_do_not_use_builtin_random_in_tests
)

# Initialize a variable to track if any check fails
any_check_failed=0

# Loop over all checks
for check in "${checks[@]}"; do
$check
result=$?
if [ $result -ne 0 ]; then
echo -e "${RED}Check $check FAILED${NC}"
any_check_failed=1
else
echo -e "${GREEN}Check $check PASSED${NC}"
fi
done

# Exit with code 0 if no check failed, otherwise exit with code 1
if [ $any_check_failed -eq 0 ]; then
echo -e "${GREEN}All checks PASSED${NC}"
exit 0
else
echo -e "${RED}Some checks FAILED${NC}"
exit 1
fi
4 changes: 1 addition & 3 deletions tests/p2p/test_double_spending.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import random

from hathor.crypto.util import decode_address
from tests import unittest
from tests.utils import add_blocks_unlock_reward, add_new_blocks, add_new_tx
Expand All @@ -21,7 +19,7 @@ def _add_new_transactions(self, manager, num_txs):
txs = []
for _ in range(num_txs):
address = self.get_address(0)
value = random.choice([5, 10, 15, 20])
value = self.rng.choice([5, 10, 15, 20])
tx = add_new_tx(manager, address, value)
txs.append(tx)
return txs
Expand Down
20 changes: 6 additions & 14 deletions tests/p2p/test_split_brain.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import random

import pytest
from mnemonic import Mnemonic

Expand All @@ -16,16 +14,10 @@ class BaseHathorSplitBrainTestCase(unittest.TestCase):

def setUp(self):
super().setUp()

# import sys
# from twisted.python import log
# log.startLogging(sys.stdout)

# self.set_random_seed(0)

from hathor.transaction.genesis import _get_genesis_transactions_unsafe

first_timestamp = min(tx.timestamp for tx in _get_genesis_transactions_unsafe(None))
self.clock.advance(first_timestamp + random.randint(3600, 120*24*3600))
self.clock.advance(first_timestamp + self.rng.randint(3600, 120*24*3600))

self.network = 'testnet'

Expand All @@ -40,7 +32,7 @@ def create_peer(self, network, unlock_wallet=True):
# Don't use it anywhere else. It is unsafe to generate mnemonic words like this.
# It should be used only for testing purposes.
m = Mnemonic('english')
words = m.to_mnemonic(bytes(random.randint(0, 255) for _ in range(32)))
words = m.to_mnemonic(bytes(self.rng.randint(0, 255) for _ in range(32)))
wallet.unlock(words=words, tx_storage=manager.tx_storage)
return manager

Expand All @@ -60,9 +52,9 @@ def test_split_brain(self):
add_new_block(manager2, advance_clock=1)
add_blocks_unlock_reward(manager2)
self.clock.advance(10)
for _ in range(random.randint(3, 10)):
add_new_transactions(manager1, random.randint(2, 4), advance_clock=1)
add_new_transactions(manager2, random.randint(3, 7), advance_clock=1)
for _ in range(self.rng.randint(3, 10)):
add_new_transactions(manager1, self.rng.randint(2, 4), advance_clock=1)
add_new_transactions(manager2, self.rng.randint(3, 7), advance_clock=1)
add_new_double_spending(manager1)
add_new_double_spending(manager2)
self.clock.advance(10)
Expand Down
4 changes: 1 addition & 3 deletions tests/p2p/test_sync.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import random

from twisted.python.failure import Failure

from hathor.conf import HathorSettings
Expand Down Expand Up @@ -53,7 +51,7 @@ def _add_new_transactions(self, num_txs):
txs = []
for _ in range(num_txs):
address = self.get_address(0)
value = random.choice([5, 10, 50, 100, 120])
value = self.rng.choice([5, 10, 50, 100, 120])
tx = self._add_new_tx(address, value)
txs.append(tx)
return txs
Expand Down
4 changes: 1 addition & 3 deletions tests/p2p/test_sync_mempool.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import random

from hathor.crypto.util import decode_address
from hathor.graphviz import GraphvizVisualizer
from hathor.simulator import FakeConnection
Expand Down Expand Up @@ -43,7 +41,7 @@ def _add_new_transactions(self, num_txs):
txs = []
for _ in range(num_txs):
address = self.get_address(0)
value = random.choice([5, 10, 50, 100, 120])
value = self.rng.choice([5, 10, 50, 100, 120])
tx = self._add_new_tx(address, value)
txs.append(tx)
return txs
Expand Down
6 changes: 5 additions & 1 deletion tests/unittest.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import os
import secrets
import shutil
import tempfile
import time
Expand Down Expand Up @@ -99,6 +100,7 @@ class TestCase(unittest.TestCase):
_enable_sync_v1: bool
_enable_sync_v2: bool
use_memory_storage: bool = USE_MEMORY_STORAGE
seed_config: Optional[int] = None

def setUp(self):
_set_test_mode(TestMode.TEST_ALL_WEIGHT)
Expand All @@ -108,7 +110,9 @@ def setUp(self):
self.clock.advance(time.time())
self.log = logger.new()
self.reset_peer_id_pool()
self.rng = Random()
self.seed = secrets.randbits(64) if self.seed_config is None else self.seed_config
self.log.debug('set seed', seed=self.seed)
self.rng = Random(self.seed)
self._pending_cleanups = []

def tearDown(self):
Expand Down