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

Do not reset DB in CI tests if not needed #44084

Merged
merged 1 commit into from
Nov 16, 2024

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Nov 16, 2024

The #43979 refactoring of tests caused unnecessary database reset attempts in tests that did not require it or had no database set.

This caused unnecessary airflow db reset in collection-only tests with removed non-ARM packages, but also it caused the error printed in non-DB tests:

Resetting the DB

[2024-11-16T03:50:37.812+0000] {cli_parser.py:67} ERROR - Failed to load CLI commands from executor: LocalExecutor
Traceback (most recent call last):
  File "/opt/airflow/airflow/cli/cli_parser.py", line 64, in <module>
    executor, _ = ExecutorLoader.import_executor_cls(executor_name)
  File "/opt/airflow/airflow/executors/executor_loader.py", line 285, in import_executor_cls
    return _import_and_validate(executor_name.module_path), executor_name.connector_source
  File "/opt/airflow/airflow/executors/executor_loader.py", line 282, in _import_and_validate
    cls.validate_database_executor_compatibility(executor)
  File "/opt/airflow/airflow/executors/executor_loader.py", line 327, in validate_database_executor_compatibility
    if engine.dialect.name == "sqlite":
AttributeError: 'NoneType' object has no attribute 'dialect'
[2024-11-16T03:50:37.813+0000] {cli_parser.py:68} ERROR - Ensure all dependencies are met and try again. If using a Celery based executor install a 3.3.0+ version of the Celery provider. If using a Kubernetes executor, install a 7.4.0+ version of the CNCF provider
Traceback (most recent call last):
  File "/usr/local/bin/airflow", line 8, in <module>
    sys.exit(main())
  File "/opt/airflow/airflow/__main__.py", line 62, in main
    args.func(args)
  File "/opt/airflow/airflow/cli/cli_config.py", line 49, in command
    return func(*args, **kwargs)
  File "/opt/airflow/airflow/utils/providers_configuration_loader.py", line 55, in wrapped_function
    return func(*args, **kwargs)
  File "/opt/airflow/airflow/cli/commands/db_command.py", line 63, in resetdb
    print(f"DB: {settings.engine.url!r}")
AttributeError: 'NoneType' object has no attribute 'url'

Database has been reset

The fix is to add --no-db-reset in collection tests and force db_reset = False in case skip_db_tests is set to True.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Choose a reason for hiding this comment

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

Copilot reviewed 10 out of 18 changed files in this pull request and generated no suggestions.

Files not reviewed (8)
  • dev/breeze/doc/images/output_shell.txt: Language not supported
  • dev/breeze/doc/images/output_start-airflow.txt: Language not supported
  • dev/breeze/doc/images/output_testing_core-integration-tests.txt: Language not supported
  • dev/breeze/doc/images/output_testing_core-tests.txt: Language not supported
  • dev/breeze/doc/images/output_testing_providers-integration-tests.txt: Language not supported
  • dev/breeze/doc/images/output_testing_providers-tests.txt: Language not supported
  • dev/breeze/doc/images/output_testing_system-tests.txt: Language not supported
  • scripts/ci/testing/run_unit_tests.sh: Language not supported
Comments skipped due to low confidence (1)

dev/breeze/src/airflow_breeze/commands/testing_commands.py:1116

  • Ensure that the new behavior introduced by 'db_reset=db_reset if not skip_db_tests else False' is covered by tests.
db_reset=db_reset if not skip_db_tests else False
@potiuk potiuk requested a review from rawwar November 16, 2024 04:08
@potiuk potiuk force-pushed the do-not-reset-db-in-non-db-tests branch from 6bf3be4 to ee8f039 Compare November 16, 2024 04:17
@potiuk potiuk requested a review from jscheffl November 16, 2024 10:21
The apache#43979 refactoring of tests caused unnecessary database reset
attempts in tests that did not require it or had no database set.

This caused unnecessary `airflow db reset` in collection-only tests
with removed non-ARM packages, but also it caused the error printed
in non-DB tests:

```
Resetting the DB

[2024-11-16T03:50:37.812+0000] {cli_parser.py:67} ERROR - Failed to load CLI commands from executor: LocalExecutor
Traceback (most recent call last):
  File "/opt/airflow/airflow/cli/cli_parser.py", line 64, in <module>
    executor, _ = ExecutorLoader.import_executor_cls(executor_name)
  File "/opt/airflow/airflow/executors/executor_loader.py", line 285, in import_executor_cls
    return _import_and_validate(executor_name.module_path), executor_name.connector_source
  File "/opt/airflow/airflow/executors/executor_loader.py", line 282, in _import_and_validate
    cls.validate_database_executor_compatibility(executor)
  File "/opt/airflow/airflow/executors/executor_loader.py", line 327, in validate_database_executor_compatibility
    if engine.dialect.name == "sqlite":
AttributeError: 'NoneType' object has no attribute 'dialect'
[2024-11-16T03:50:37.813+0000] {cli_parser.py:68} ERROR - Ensure all dependencies are met and try again. If using a Celery based executor install a 3.3.0+ version of the Celery provider. If using a Kubernetes executor, install a 7.4.0+ version of the CNCF provider
Traceback (most recent call last):
  File "/usr/local/bin/airflow", line 8, in <module>
    sys.exit(main())
  File "/opt/airflow/airflow/__main__.py", line 62, in main
    args.func(args)
  File "/opt/airflow/airflow/cli/cli_config.py", line 49, in command
    return func(*args, **kwargs)
  File "/opt/airflow/airflow/utils/providers_configuration_loader.py", line 55, in wrapped_function
    return func(*args, **kwargs)
  File "/opt/airflow/airflow/cli/commands/db_command.py", line 63, in resetdb
    print(f"DB: {settings.engine.url!r}")
AttributeError: 'NoneType' object has no attribute 'url'

Database has been reset
```

The fix is to add `--no-db-reset` in collection tests and force
db_reset = False in case `skip_db_tests` is set to True.
@potiuk potiuk force-pushed the do-not-reset-db-in-non-db-tests branch from ee8f039 to b622202 Compare November 16, 2024 10:23
@potiuk potiuk merged commit 8c53a20 into apache:main Nov 16, 2024
95 checks passed
@potiuk potiuk deleted the do-not-reset-db-in-non-db-tests branch November 16, 2024 11:24
kandharvishnu pushed a commit to kandharvishnu/airflow that referenced this pull request Nov 19, 2024
The apache#43979 refactoring of tests caused unnecessary database reset
attempts in tests that did not require it or had no database set.

This caused unnecessary `airflow db reset` in collection-only tests
with removed non-ARM packages, but also it caused the error printed
in non-DB tests:

```
Resetting the DB

[2024-11-16T03:50:37.812+0000] {cli_parser.py:67} ERROR - Failed to load CLI commands from executor: LocalExecutor
Traceback (most recent call last):
  File "/opt/airflow/airflow/cli/cli_parser.py", line 64, in <module>
    executor, _ = ExecutorLoader.import_executor_cls(executor_name)
  File "/opt/airflow/airflow/executors/executor_loader.py", line 285, in import_executor_cls
    return _import_and_validate(executor_name.module_path), executor_name.connector_source
  File "/opt/airflow/airflow/executors/executor_loader.py", line 282, in _import_and_validate
    cls.validate_database_executor_compatibility(executor)
  File "/opt/airflow/airflow/executors/executor_loader.py", line 327, in validate_database_executor_compatibility
    if engine.dialect.name == "sqlite":
AttributeError: 'NoneType' object has no attribute 'dialect'
[2024-11-16T03:50:37.813+0000] {cli_parser.py:68} ERROR - Ensure all dependencies are met and try again. If using a Celery based executor install a 3.3.0+ version of the Celery provider. If using a Kubernetes executor, install a 7.4.0+ version of the CNCF provider
Traceback (most recent call last):
  File "/usr/local/bin/airflow", line 8, in <module>
    sys.exit(main())
  File "/opt/airflow/airflow/__main__.py", line 62, in main
    args.func(args)
  File "/opt/airflow/airflow/cli/cli_config.py", line 49, in command
    return func(*args, **kwargs)
  File "/opt/airflow/airflow/utils/providers_configuration_loader.py", line 55, in wrapped_function
    return func(*args, **kwargs)
  File "/opt/airflow/airflow/cli/commands/db_command.py", line 63, in resetdb
    print(f"DB: {settings.engine.url!r}")
AttributeError: 'NoneType' object has no attribute 'url'

Database has been reset
```

The fix is to add `--no-db-reset` in collection tests and force
db_reset = False in case `skip_db_tests` is set to True.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants