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

🧹 Introduce Python/HTML tests #6052

Merged
merged 18 commits into from
Dec 19, 2024
Merged

🧹 Introduce Python/HTML tests #6052

merged 18 commits into from
Dec 19, 2024

Conversation

rix0rrr
Copy link
Collaborator

@rix0rrr rix0rrr commented Dec 17, 2024

Motivation

Because it is hard to write Cypress tests, people often don't do it, which increases the risk of regressions in the project. In this PR, I'm making some changes to make it easier to write tests on the Flask code in Python in the future, which should hopefully make it easier to write tests overall. I'm thinking that Cypress tests have the following downsides which lead us to not writing very many of them:

  • Setup is awkward: to test a UI feature, you need the database in a situation where the tested UI element would be relevant. This means a program in state X, or a user in state Y. It is awkward to use the UI in Cypress to set up a database this way.
  • Wait problems: complex interactions often have multiple steps, and it is easy to have race conditions in Cypress tests, where tests work on your machine but fail on the build server because of timing issues. Instead, in Python tests we'll make assertions on the HTML sent to the client, which is either good or bad in one shot.

Testing in Python is not a panacea: we will obviously not be able to test anything that involves JavaScript, and we'll be asserting on the HTML only which of course tests nothing about presentation or element visibility.

I'm not proposing to replace all our tests with Python/HTML tests, just to get more coverage more easily this way.

If nothing else, the changes in this PR will help clean up the code base a little 😉.

Motivation

Previous, we used to create a global

app = Flask(...)

object, and initialize it over the course of the app.py file.

This works well to get a server up and running, but it makes it harder to test the code: for tests, you want fresh objects for every run.

In this PR, move the initialization code into a create_app() function, so that we can create fresh instances of a Flask object on demand. Also write some functions to clear the in-memory database state, and write some Python/HTML tests to show what it looks like.

This suite can also replace our current out-of-process "e2e" tests (tests_e2e.py) because it is ~the same, but hopefully easier to write.

rix0rrr and others added 5 commits December 17, 2024 09:23
Motivation
----------

Because it is hard to write Cypress tests, people often don't do it,
which increases the risk of regressions in the project. In this PR, I'm
making some changes to make it easier to write tests on the Flask code
in Python in the future, which should hopefully make it easier to
write tests overall. I'm thinking that Cypress tests have the following
downsides which lead us to not writing very many of them:

- Setup is awkward: to test a UI feature, you need the database in a
  situation where the tested UI element would be relevant. This means
  a program in state X, or a user in state Y. It is awkward to use the
  UI in Cypress to set up a database this way.
- Wait problems: complex interactions often have multiple steps, and it
  is easy to have race conditions in Cypress tests, where tests work
  on your machine but fail on the build server because of timing issues.
  Instead, in Python tests we'll make assertions on the HTML sent to
  the client, which is either good or bad in one shot.

Testing in Python is not a panacea: we will obviously not be able to
test anything that involves JavaScript, and we'll be asserting on
the HTML only which of  course tests nothing about presentation or
element visibility.

I'm not proposing to replace all our tests with Python/HTML tests, just
to get more coverage more easily this way.

If nothing else, the changes in this PR will help clean up the code
base a little 😉.

Motivation
----------

Previous, we used to create a global

```py
app = Flask(...)
```

object, and initialize it over the course of the `app.py` file.

This works well to get a server up and running, but it makes it harder
to test the code: for tests, you want fresh objects for every run.

In this PR, move the initialization code into a `create_app()` function,
so that we can create fresh instances of a `Flask` object on demand.
Also write some functions to clear the in-memory database state,
and write some Python/HTML tests to show what it looks like.
Copy link
Collaborator

@boryanagoncharenko boryanagoncharenko left a comment

Choose a reason for hiding this comment

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

Awesome refactoring.

Just want to point out that the biggest issue with adding cypress tests is that the cypress suite takes 20-25 minutes to run. Several months ago this has been brought up and it was clear that the execution time was considered to be very long. I am convinced that the python tests will help in this regard.

A small comment on the implementation, when I run the validate-tests script, I get the following 3 messages which I did not get before:

[2024-12-19 12:10:10,810] log_fetcher.py from_env 28 <WARNING>: Unable to initialize Athena client (missing AWS_ACCESS_KEY_ID or AWS_SECRET_ACCESS_KEY)
[2024-12-19 12:10:10,811] aws_helpers.py s3_parselog_transmitter_from_env 31 <WARNING>: Unable to initialize S3 parse logger (missing AWS_ACCESS_KEY_ID or AWS_SECRET_ACCESS_KEY)
[2024-12-19 12:10:10,811] aws_helpers.py s3_querylog_transmitter_from_env 20 <WARNING>: Unable to initialize S3 querylogger (missing AWS_ACCESS_KEY_ID or AWS_SECRET_ACCESS_KEY)

I noticed that the first message is produced by dead code. The athena_client and the log_fetcher are only used for the enpoints /logs/results and /logs/query which have no incoming traffic.

Let me know if you want to address the warnings.

@rix0rrr
Copy link
Collaborator Author

rix0rrr commented Dec 19, 2024

I'll remove those lines. I don't think they add a lot of value anymore.

Copy link
Contributor

mergify bot commented Dec 19, 2024

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit ffdf595 into main Dec 19, 2024
11 checks passed
@mergify mergify bot deleted the use-factory-function branch December 19, 2024 16:52
rix0rrr added a commit that referenced this pull request Dec 27, 2024
The pagination links in various pages were using
`url_for('programs_page')`, which builds a URL to a route on the
global `app` object.

Since #6052 we don't have a global app object anymore. Instead, we use a
global Blueprint called `app` instead.

In this PR, make all `url_for` calls in the same `app.py` file
Blueprint-relative by prepending a period: `url_for('.programs_page')`,
and make the `url_for` in the teachers page that points to a user's
programs page (probably legacy and unused?) absolute by prepending
the full Blueprint name: `url_for('app.programs_page')`.

Add tests to make sure that the page renders successfully.
mergify bot pushed a commit that referenced this pull request Dec 27, 2024
The pagination links in various pages were using `url_for('programs_page')`, which builds a URL to a route on the global `app` object.

Since #6052 we don't have a global app object anymore. Instead, we use a global Blueprint called `app` instead.

In this PR, make all `url_for` calls in the same `app.py` file Blueprint-relative by prepending a period: `url_for('.programs_page')`, and make the `url_for` in the teachers page that points to a user's programs page (probably legacy and unused?) absolute by prepending the full Blueprint name: `url_for('app.programs_page')`.

Add tests to make sure that the page renders successfully.

**How to test**

Automatic tests have been added, so if they pass we should be good.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants