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

Make the test correspond to the new beam size handling. #1379

Merged
merged 5 commits into from
Aug 30, 2024

Conversation

beteva
Copy link
Member

@beteva beteva commented Aug 28, 2024

  1. Change the type to be str and not int
  2. Define aperture and not slits to be the default beam size definer.

Needs the mxcubecore fixes. Partly solves #1370.

@elmjag
Copy link
Contributor

elmjag commented Aug 29, 2024

I tested this branch with latest mxcubecore (v1.145.0), I needed to make this change:

diff --git a/test/test_beamline_routes.py b/test/test_beamline_routes.py
index ccbb2c52..c1143e77 100644
--- a/test/test_beamline_routes.py
+++ b/test/test_beamline_routes.py
@@ -21,6 +21,7 @@ def test_beamline_get_all_attribute(client):
 
     expected = [
         "beam",
+        "beam.aperture",
         "cryo",
         "data_publisher",
         "detector",

With the patch above, all tests pass. \o/

Also, I guess we need to change the earliest required mxcubecore version to v1.145.0 here: https://github.com/mxcube/mxcubeweb/blob/develop/pyproject.toml#L44

@elmjag
Copy link
Contributor

elmjag commented Aug 30, 2024

nice, now the tests are passing for me!

@marcus-oscarsson marcus-oscarsson merged commit 4e885e6 into develop Aug 30, 2024
13 checks passed
@marcus-oscarsson marcus-oscarsson deleted the beam_definer branch August 30, 2024 05:50
@fabcor-maxiv
Copy link
Contributor

I'm sorry to have to bring this up, but I believe we should avoid this kind of things:

* 4e885e64 Also update requirements
* 7f2f962f Upgrade lock file
* fe55ba3d Upgrade mxcubecore version.

This should have been one single commit.

I did spend some time working on the pre-commit hooks to prevent exactly this. Are they not working as expected? I can probably fix them if it is the case and you let me know. : )

@beteva
Copy link
Member Author

beteva commented Aug 30, 2024

I did spend some time working on the pre-commit hooks to prevent exactly this. Are they not working as expected? I can probably fix them if it is the case and you let me know. : )

You are absolutely right. I've just forgotten to run the pre-comit. It works really well indeed.

@fabcor-maxiv
Copy link
Contributor

The issue is that we now have some commits in the repository that we know 100% for sure that they have defects, and we perfectly knew that they had defects before merging them. The CI checks told us so.

Forgetting the pre-commit hooks, is no problem at all, but...
If the GitHub CI checks fail, it should NEVER be solved by adding more commits, it should be solved by fixing the commit itself (i.e. amend the commit).

Squashing the commits when merging the pull request could help getting rid of the defective commits as well of course, even though that would not be my preference. My preference is 1 single commit per pull request, which then obviously avoids this kind of issues.

I picked this particular example I highlighted above because it is relatively harmless, but still...
Which commit should I revert if for whatever reason I want to roll-back to the previous mxcubecore version?

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.

4 participants