Skip to content

Commit 1f20066

Browse files
authored
Fix oppia#22564: standardize script argument delimiters, and add pytest integration to run_backend_tests.py (oppia#23927)
* Add infrastructure setup for pytest support. * Fix lint * Fix lint * Singleton pattern * more fixes * mypy * add tests * fix lint check * more ifxes * Drop unneeded stuff * fix singleton impl * Address bug * Fix test * drop type-ignore * Drop unneeded changes. * Fix imports * Add conftest_test to backend test shards * Add --use-pytest flag in run_backend_tests.py * Fix docstring * Add test for coverage * Consolidate tests * Fix underscore/hyphen discrepancy. * Fix all underscore / hyphen issues * simplify
1 parent 86a7408 commit 1f20066

File tree

13 files changed

+473
-25
lines changed

13 files changed

+473
-25
lines changed

.github/actions/run-a-specific-acceptance-test/action.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ runs:
3939
VIDEO_RECORDING_IS_ENABLED=${{ inputs.enable-screen-recording && '1' || '0' }}
4040
xvfb-run -a --server-args="-screen 0, 1920x1080x24"
4141
python -m scripts.run_acceptance_tests
42-
--skip-build
42+
--skip_build
4343
--suite=${{ inputs.test-suite }}
4444
${{ inputs.viewport-type == 'mobile' && '--mobile' || '' }}
4545
--prod_env

.github/workflows/full_stack_tests.yml

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -119,8 +119,8 @@ jobs:
119119
set -o pipefail;
120120
VIDEO_RECORDING_IS_ENABLED=0
121121
xvfb-run -a --server-args="-screen 0, 1285x1000x24"
122-
python -m scripts.run_e2e_tests --skip-install
123-
--skip-build --suite=${{ matrix.suite.name }} --prod_env --server_log_level=info
122+
python -m scripts.run_e2e_tests --skip_install
123+
--skip_build --suite=${{ matrix.suite.name }} --prod_env --server_log_level=info
124124
| tee e2e_test_${{ steps.generate_suite_name_for_artifacts.outputs.MODIFIED_SUITE_NAME }}.log
125125
- name: Upload E2E test logs
126126
if: ${{ failure() }}
@@ -134,8 +134,8 @@ jobs:
134134
set -o pipefail;
135135
VIDEO_RECORDING_IS_ENABLED=1
136136
xvfb-run -a --server-args="-screen 0, 1285x1000x24"
137-
python -m scripts.run_e2e_tests --skip-install
138-
--skip-build --suite=${{ matrix.suite.name }} --prod_env --server_log_level=info
137+
python -m scripts.run_e2e_tests --skip_install
138+
--skip_build --suite=${{ matrix.suite.name }} --prod_env --server_log_level=info
139139
| tee e2e_test_retry_${{ steps.generate_suite_name_for_artifacts.outputs.MODIFIED_SUITE_NAME }}.log
140140
- name: Upload E2E retry test logs
141141
if: ${{ failure() }}

AGENTS.md

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ environment setup and troubleshooting.
3030

3131
- Run backend unit tests locally:
3232

33-
python -m scripts.run_backend_tests --test-target <path_or_testname>
33+
python -m scripts.run_backend_tests --test_target <path_or_testname>
3434

3535
(This is the same test runner used by CI.)
3636

@@ -39,6 +39,9 @@ environment setup and troubleshooting.
3939
python -m scripts.linters.run_lint_checks
4040
npx prettier --check .
4141

42+
Note: You can use the `--skip_install` flag with test scripts to
43+
skip dependency installation if you've already installed them recently.
44+
4245
Project-specific conventions and patterns (discoverable in the tree):
4346

4447
- Backend structure: controllers provide request handlers; business logic lives
@@ -57,6 +60,26 @@ Project-specific conventions and patterns (discoverable in the tree):
5760
(e.g. `node-16.13.0`). In general we prefer using the pinned versions in
5861
CI and in local development so that we can keep the environment consistent.
5962

63+
Code quality and testing standards:
64+
65+
- Test coverage: Maintain 100% line and branch coverage for all new or modified
66+
code. Always add tests for new functionality.
67+
- Type safety: Never use `Any` or `unknown` types in TypeScript. Use specific,
68+
well-defined types.
69+
- Documentation: Document anything that is not obvious. Add comments explaining
70+
the "why" behind non-obvious decisions, not just the "what".
71+
- Pylint disables: Never use whole-file pylint disables (e.g., at the top of a
72+
file). Disable specific checks only on the lines that need them, with a
73+
comment explaining why the disable is necessary.
74+
- Lint errors: Custom lint scripts live in the codebase itself (under
75+
`scripts/linters/`). Search for the error message in the codebase to find the
76+
script that's checking for it and understand what it's looking for.
77+
- Main branch: The main Oppia branch is `develop`, not `main` or `master`.
78+
- Communication: Never present speculation as fact. If you're uncertain about
79+
behavior, document format, or best practices, clearly state at the outset
80+
that you're making an educated guess or recommendation rather than citing
81+
confirmed documentation.
82+
6083
Integration points / external dependencies:
6184

6285
- Google App Engine concepts (datastore, platform services).
@@ -71,7 +94,7 @@ Integration points / external dependencies:
7194
Small examples for agents to be effective quickly:
7295

7396
-- To run a focused backend test for a file locally:
74-
`python -m scripts.run_backend_tests --test-target <path_or_testname>`.
97+
`python -m scripts.run_backend_tests --test_target <path_or_testname>`.
7598
-- When editing frontend deps or build config locally: install Node 16 (nvm),
7699
then run `yarn install --pure-lockfile` from `assets/` and follow the steps
77100
for running frontend scripts described on the wiki.

scripts/linters/run_lint_checks.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@
4242
5. To lint a specific list of file extensions. Separate file
4343
extensions by spaces
4444
python -m scripts.linters.run_lint_checks
45-
--only-check-file-extensions py js
45+
--only_check_file_extensions py js
4646
4747
6. To run a shard of the lint tests
4848
python -m scripts.linters.run_lint_checks --shard shard_name
@@ -113,7 +113,7 @@
113113
action='store_true',
114114
)
115115
_PARSER.add_argument(
116-
'--only-check-file-extensions',
116+
'--only_check_file_extensions',
117117
nargs='+',
118118
choices=['html', 'css', 'js', 'ts', 'py', 'other'],
119119
help='specific file extensions to be linted. Space separated list. '

scripts/linters/run_lint_checks_test.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,7 @@ def test_main_with_only_check_file_extensions_arg(self) -> None:
293293
run_lint_checks.main(
294294
args=[
295295
'--path=%s' % VALID_TS_FILEPATH,
296-
'--only-check-file-extensions=ts',
296+
'--only_check_file_extensions=ts',
297297
]
298298
)
299299
self.assertFalse(all_checks_passed(self.linter_stdout))
@@ -305,7 +305,7 @@ def test_main_with_only_check_file_extensions_arg_with_js_ts_options(
305305
run_lint_checks.main(
306306
args=[
307307
'--path=%s' % VALID_TS_FILEPATH,
308-
'--only-check-file-extensions',
308+
'--only_check_file_extensions',
309309
'ts',
310310
'js',
311311
]
@@ -325,7 +325,7 @@ def test_get_all_files_in_directory(self) -> None:
325325
run_lint_checks.main(
326326
args=[
327327
'--path=scripts/linters/',
328-
'--only-check-file-extensions=ts',
328+
'--only_check_file_extensions=ts',
329329
]
330330
)
331331

scripts/pre_push_hook.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@
8686
'-m',
8787
'scripts.run_backend_tests',
8888
'--ignore_coverage',
89-
'--skip-install',
89+
'--skip_install',
9090
]
9191
BACKEND_ASSOCIATED_TEST_FILE_CHECK_CMD: Final = [
9292
PYTHON_CMD,

scripts/run_acceptance_tests.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838
)
3939

4040
_PARSER.add_argument(
41-
'--skip-build',
41+
'--skip_build',
4242
help='If true, skips building files. The default value is false.',
4343
action='store_true',
4444
)

scripts/run_acceptance_tests_test.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,7 @@ def test_start_tests_skip_build(self) -> None:
453453

454454
with self.compile_test_ts_files_swap:
455455
run_acceptance_tests.main(
456-
args=['--suite', 'testSuite', '--skip-build']
456+
args=['--suite', 'testSuite', '--skip_build']
457457
)
458458

459459
def test_start_tests_in_jasmine(self) -> None:

scripts/run_backend_tests.py

Lines changed: 134 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@
6161

6262
from core import utils
6363

64+
import pytest
6465
from typing import Dict, Final, List, Optional, Tuple, cast
6566

6667
from . import (
@@ -155,11 +156,17 @@
155156
action='store_true',
156157
)
157158
_PARSER.add_argument(
158-
'--skip-install',
159+
'--skip_install',
159160
help='optional; if specified, skips the installation of '
160161
'third party libraries',
161162
action='store_true',
162163
)
164+
_PARSER.add_argument(
165+
'--use_pytest',
166+
help='optional; if specified, uses pytest as the test runner instead of '
167+
'the default gae_suite runner',
168+
action='store_true',
169+
)
163170

164171

165172
def run_shell_cmd(
@@ -443,6 +450,113 @@ def check_test_results(
443450
return total_count, total_errors, total_failures, time_report
444451

445452

453+
def convert_args_to_pytest(parsed_args: argparse.Namespace) -> List[str]:
454+
"""Convert run_backend_tests.py arguments to pytest arguments.
455+
456+
Args:
457+
parsed_args: argparse.Namespace. Parsed command-line arguments.
458+
459+
Returns:
460+
list(str). List of pytest command-line arguments.
461+
462+
Raises:
463+
Exception. The shard configuration in backend_test_shards.json doesn't
464+
match the actual test files on the filesystem when using the
465+
--test_shard flag. This can happen when (a) a test file listed in
466+
the JSON shards file doesn't exist, (b) a test file exists but
467+
isn't listed in any shard, or (c) a test file is listed in
468+
multiple shards.
469+
"""
470+
pytest_args = []
471+
472+
# Add verbosity flag.
473+
pytest_args.append('-v' if parsed_args.verbose else '-q')
474+
475+
# Add coverage flags if requested.
476+
if parsed_args.generate_coverage_report:
477+
pytest_args.extend(['--cov=.', '--cov-report=term-missing'])
478+
if not parsed_args.ignore_coverage:
479+
pytest_args.append('--cov-fail-under=100')
480+
481+
# Handle test selection.
482+
if parsed_args.test_targets:
483+
# Convert dot notation to pytest path notation.
484+
for test_target in parsed_args.test_targets.split(','):
485+
# Check if this is a specific test (has _test. in it).
486+
# Since _test always appears as a suffix on module names, we can
487+
# simply split on '.' and find the part ending with '_test'.
488+
if '_test.' in test_target:
489+
# Find the module ending with _test.
490+
parts = test_target.split('.')
491+
test_idx = next(
492+
i for i, part in enumerate(parts) if part.endswith('_test')
493+
)
494+
495+
# Original format: module.path_test.ClassName(.method_name).
496+
# Convert to: module/path_test.py::ClassName(::method_name).
497+
test_path = '%s.py::%s' % (
498+
'/'.join(parts[: test_idx + 1]),
499+
'::'.join(parts[test_idx + 1 :]),
500+
)
501+
502+
pytest_args.append(test_path)
503+
elif test_target.endswith('_test'):
504+
# Just a test module.
505+
test_path = test_target.replace('.', '/') + '.py'
506+
pytest_args.append(test_path)
507+
else:
508+
# Not a test file, add _test suffix.
509+
test_path = test_target.replace('.', '/') + '_test.py'
510+
pytest_args.append(test_path)
511+
elif parsed_args.test_path:
512+
pytest_args.append(parsed_args.test_path)
513+
elif parsed_args.test_shard:
514+
# Get all test targets from shard and convert to paths.
515+
validation_error = check_shards_match_tests(include_load_tests=True)
516+
if validation_error:
517+
raise Exception(validation_error)
518+
all_test_targets = get_all_test_targets_from_shard(
519+
parsed_args.test_shard
520+
)
521+
for test_target in all_test_targets:
522+
test_path = test_target.replace('.', '/') + '.py'
523+
pytest_args.append(test_path)
524+
elif parsed_args.run_on_changed_files_in_branch:
525+
changed_files = git_changes_utils.get_changed_python_test_files()
526+
for test_target in changed_files:
527+
test_path = test_target.replace('.', '/') + '.py'
528+
pytest_args.append(test_path)
529+
else:
530+
# Run all tests.
531+
if parsed_args.exclude_load_tests:
532+
pytest_args.append('--ignore=core/tests/load_tests')
533+
# Default: run all tests in current directory.
534+
pytest_args.append('.')
535+
536+
return pytest_args
537+
538+
539+
def run_tests_with_pytest(parsed_args: argparse.Namespace) -> int:
540+
"""Run tests using pytest instead of gae_suite.
541+
542+
Args:
543+
parsed_args: argparse.Namespace. Parsed command-line arguments.
544+
545+
Returns:
546+
int. Exit code from pytest (0 for success, non-zero for failure).
547+
"""
548+
pytest_args = convert_args_to_pytest(parsed_args)
549+
550+
print('Running tests with pytest...')
551+
print('Pytest arguments: %s' % ' '.join(pytest_args))
552+
print('')
553+
554+
# Run pytest with the converted arguments.
555+
exit_code = pytest.main(pytest_args)
556+
557+
return exit_code
558+
559+
446560
def main(args: Optional[List[str]] = None) -> None:
447561
"""Run the tests."""
448562
parsed_args = _PARSER.parse_args(args=args)
@@ -466,6 +580,25 @@ def main(args: Optional[List[str]] = None) -> None:
466580
raise Exception('The delimiter in test_path should be a slash (/)')
467581
if not parsed_args.skip_install:
468582
install_third_party_libs.main()
583+
584+
# If --use_pytest flag is set, delegate to pytest and return early.
585+
if parsed_args.use_pytest:
586+
with contextlib.ExitStack() as stack:
587+
stack.enter_context(
588+
servers.managed_cloud_datastore_emulator(clear_datastore=True)
589+
)
590+
stack.enter_context(servers.managed_redis_server())
591+
592+
# Run tests with pytest.
593+
exit_code = run_tests_with_pytest(parsed_args)
594+
595+
if exit_code != 0:
596+
raise Exception('Tests failed with exit code %d' % exit_code)
597+
598+
print('')
599+
print('Done!')
600+
return
601+
469602
with contextlib.ExitStack() as stack:
470603
stack.enter_context(
471604
servers.managed_cloud_datastore_emulator(clear_datastore=True)

0 commit comments

Comments
 (0)