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

BC-4100 - Create Demo School Endpoint #651

Closed
wants to merge 14 commits into from

Conversation

hoeppner-dataport
Copy link
Contributor

@hoeppner-dataport hoeppner-dataport commented Sep 25, 2023

Description

New Feature-Flag FEATURE_CYPRESS_SETUP_ENABLED to allow setting up demo-schools, -users, -courses etc.

Links to Tickets or other pull requests

https://ticketsystem.dbildungscloud.de/browse/BC-4100
hpi-schul-cloud/schulcloud-server#4365
hpi-schul-cloud/nuxt-client#2827

Changes

Datasecurity

Deployment

New Repos, NPM pakages or vendor scripts

Screenshots of UI changes

Approval for review

  • All points were discussed with the ticket creator, support-team or product owner. The code upholds all quality guidelines from the PR-template.

Notice: Please remove the WIP label if the PR is ready to review, otherwise nobody will review it.

ansible/group_vars/all/x_ingress.yml Outdated Show resolved Hide resolved
@@ -882,6 +882,11 @@ configuration_all:
server: true
client: false
nuxtclient: false
FEATURE_CYPRESS_SETUP_ENABLED:
value: "true"
Copy link
Contributor

@virgilchiriac virgilchiriac Oct 5, 2023

Choose a reason for hiding this comment

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

I would set this to false, and overwrite with true for dev cluster only
Is more prudent than to overwrite / disable for prod only
also, demo schule is not to be used with ref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ fixed: disabled by default and enabled for development: 757d6e7

Copy link
Member

@Loki-Afro Loki-Afro left a comment

Choose a reason for hiding this comment

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

no e2e specific code allowed, e2e tests have to be able to run against any instance, no exceptions.

e2e tests are supposed to run on every instance at any time using the same existing data. that way you can actually find bugs, testing the same software on every instance with the same data, it would be insane to find a bug which affects actual ui stuff and is not infrastructure related.

allowing to let the e2e tests reset and insert data in any database basically defeats the purpose, thus i blocked this pr

Copy link
Contributor

@Metauriel Metauriel left a comment

Choose a reason for hiding this comment

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

re @Loki-Afro

In the future it might be feasable, even useful, to create a testset of Smoketests that we can run against productive instances to ensure they are working correctly. However such tests should be minimal, and as much as possible restricted to reading operations.

Being dependent on existing data that is the same everywhere, means we can NEVER have stable tests. Manual tests can break automated tests by accidently changing data, failed tests can ruin future runs, different tests running at the same time can affect each other, overlapping runs of the same tests can conflict. It is a mess, and if we keep running the tests against preexisting data, that maybe even is used for manual tests as well, it will always be a mess. You simply cant guarantee that all instances actually have the same data, and you have to deal with a heap of additional edgecases like pagination (if the course you just created happens to be the 300th one), and you have to think about identifying objects that belong to your current test run and avoid using the same names in every execution...

Any creational user flow (and most of the important ones, such as creation and completion of tasks, are creational) needs to be tested in a circumstance that is isolated against other tests, and against other executions of the same test. You are right that the tests should always run against the same data, that means the tests have to ensure that data first. And that means the test is responsible to setup that data.

Also, the purpose of the acceptance tests is not to test the instances, its to test the code. When the data is consistent (and the tests are somewhat deterministic), its meaningless to run the same tests multiple times against the same code version on different stages. If the results on different stages are different, we need to fix that by eliminating differences between stages. When we have run the tests on main, we shouldnt have to repeat them on staging, and there should definetly never ever be a need to run them against production.

(currently we have things like the thuringia sync we can only test on staging, but we should aim to eliminate these cases and ensure EVERYTHING is testable on dev instances)

Finally, its not the purpose of the Acceptance tests to find ALL bugs that might appear in the long run. That is impossible. Its to ensure the correct functionality of critical user flows. strange behaviors that only appear in strange production data that have been around for many different versions have to be found in exploration tests, by people that have a nack for finding strange bugs, and that have time to do such tests because their time is not devoured by repetitive testing of standard user flows. That is a classic QA tasks, but they cant do it because our Acceptance tests are not stable, because we try to apply them in a way that is too unstable, too difficult to make stable, and thus too inefficient to actually implement and use.

We need to have stable acceptance tests in controlled environments, so we can reduce the time of people wasted in repetitive standard test procedures, and to eliminate stupid bugs in functionality that isnt even supposed to change, to get the critical functionality of our software stable, and free up time to move manual testing to areas that need it.

And yes it would be insane to find bugs in UI, you dont automatically test UI. You test logic, to find bugs in logic, and you abstract away visuals by using testids and such. Automatically testing UI would need screenshot tests and such stuff, and is a waste of time. Pixels can be looked at by humans.

@@ -145,6 +145,15 @@ default_ingress:
path: /api/v1/me
serviceName: api-svc
servicePort: "{{ PORT_SERVER }}"
api_v3_user-authentication-local:
Copy link
Member

Choose a reason for hiding this comment

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

update from main, update your changes accordingly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

well you are not done, just removing doesn't do the trick, it is now part of api/v3 which is public, regardless of feature flag or not this is not an option
consider moveing the stuff the the management api instead

@Metauriel Metauriel closed this Jan 26, 2024
@hoeppner-dataport hoeppner-dataport deleted the BC-4100-create-demo-school branch May 2, 2024 11:52
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.

5 participants