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

[fix] Convert case of name and URL of repsoitories to lowercase #934

Closed
wants to merge 5 commits into from
Closed

[fix] Convert case of name and URL of repsoitories to lowercase #934

wants to merge 5 commits into from

Conversation

algomaster99
Copy link
Contributor

Fixes #903

I am working on two more things which are required for this PR to be merged.

  • Write system tests.
  • Fix platform API metaclass.

@slarse I tried adding __post_init__ method inside the metaclass so that I could get iterate over the arguments there and change their case. However, the flow of the program was not calling __post_init__ so I am not sure how to go about it. I could probably override the __init__ to explicitly call __post_init__ but this did not seem like a good way to achieve it.

@slarse
Copy link
Collaborator

slarse commented Jun 28, 2021

I tried adding __post_init__ method inside the metaclass

__post_init__ is only a special method for dataclasses, it means nothing for other kinds of classes.

so that I could get iterate over the arguments there and change their case

I'm not quite sure what you mean by that in regards to _APIMeta, iterate over what arguments?

@algomaster99
Copy link
Contributor Author

I'm not quite sure what you mean by that in regards to _APIMeta, iterate over what arguments?

For example, there are methods such as assign_members which take members as a list of strings. You proposed to add a transformation function that would convert such elements to lower case. I am not sure how to write that.

I initially thought I could write a function in the _APIMeta that would iterate over the parameters of the function being called using the inspect module and convert the corresponding parameters to lowercase. But I only have access to the parameter names and not the arguments so using inspect is not the right way to go about it.

@slarse
Copy link
Collaborator

slarse commented Jul 1, 2021

I'm not quite sure what you mean by that in regards to _APIMeta, iterate over what arguments?

For example, there are methods such as assign_members which take members as a list of strings. You proposed to add a transformation function that would convert such elements to lower case. I am not sure how to write that.

I initially thought I could write a function in the _APIMeta that would iterate over the parameters of the function being called using the inspect module and convert the corresponding parameters to lowercase. But I only have access to the parameter names and not the arguments so using inspect is not the right way to go about it.

Right, now I see what you mean. The purpose of a metaclass is to alter a class definition before it is "solidified". The metaclass' __new__ method is invoked when a new class subtyping it is created (note: not when an instance of it is created, but the class itself). Therefore it's of course not possible to get any arguments for any of the methods as no method calls have yet been made; the class isn't even fully defined yet.

What's necessary there is to add a wrapper function on top of the function defined by the class. This wrapper should lowercase whatever arguments should be lowercased and then pass it on to the actual function. That's non-trivial and could be a PR of its own.

@algomaster99
Copy link
Contributor Author

This wrapper should lowercase whatever arguments should be lowercased and then pass it on to the actual function. That's non-trivial and could be a PR of its own.

Okay, we can write a decorator for it. But like you said that can be a different PR, I will just write the system tests for the changes I have introduced and then you can review.

@slarse
Copy link
Collaborator

slarse commented Jul 2, 2021

This wrapper should lowercase whatever arguments should be lowercased and then pass it on to the actual function. That's non-trivial and could be a PR of its own.

Okay, we can write a decorator for it. But like you said that can be a different PR, I will just write the system tests for the changes I have introduced and then you can review.

A decorator isn't the best solution here as it would require the implementations to use the decorator, while we want this to be automatically handled by the metaclass. An option would of course be to have the metaclass check that the correct decorator has been used to annotate the desired functions, but at that point the metaclass can just wrap the function on its own.

But, yeah, separate PR.

If you want to write system tests, use the Gitea system tests, they cost roughly 10-20 seconds per test. That's much cheaper than the GitLab system tests which consume about 1-2 minutes per tests. The integration tests (still located in tests/new_integration_tests, I should rename that...) are cheaper still, and where I personally would put tests for this functionality.

@algomaster99
Copy link
Contributor Author

If you want to write system tests, use the Gitea system tests, they cost roughly 10-20 seconds per test.

Sure. But is it assured that if something works well with Gitea, it would also work well with Github?

The integration tests (still located in tests/new_integration_tests, I should rename that...) are cheaper still, and where I personally would put tests for this functionality.

I was following this PR's format to write tests. Should I shift the tests I have written currently to new_integration_tests?

@slarse
Copy link
Collaborator

slarse commented Jul 2, 2021

If you want to write system tests, use the Gitea system tests, they cost roughly 10-20 seconds per test.

Sure. But is it assured that if something works well with Gitea, it would also work well with Github?

No, of course not. But a) the functionality in this PR is not platform dependent, b) we can't easily system test against GitHub as there's no way to setup a local instance, and c) we need to strike a balance between how expensive the tests are to run, and how important the functionality is.

I was following this PR's format to write tests. Should I shift the tests I have written currently to new_integration_tests?

Yes. The reason I added GitLab system tests for that PR was that it fixed a bug that specifically appeared on GitLab. But each GitLab system test is very expensive, so we only add one when we discover a GitLab-specific bug.

@algomaster99
Copy link
Contributor Author

Just to confirm, do you recommend writing tests in new_integration_tests instead of system_tests/gitea because this is not platform dependent?

@slarse
Copy link
Collaborator

slarse commented Jul 2, 2021

I would go with new_integration_tests. What tests precisely are you intending to write? I would test the following:

  • Running repos setup and specifying assignment names with weird case still works as expected (repos are correctly setup)
    • This should be executed with templates in the template organization
    • This should also be executed with local templates and --allow-local-templates option. There's a problem here: most *NIX file systems are case sensitive. Don't know quite what to do about that.
  • Running repos clone and specifying assignment names with weird case still works as expected (repos are correctly cloned to disk)

I realized that there is one case where this will cause strange behavior, and that's when running repos setup with --allow-local-templates.

@algomaster99
Copy link
Contributor Author

This should be executed with templates in the template organization

How does one create more repositories in the template organisation? I think I would need to create templates like TaSK-1, etc. so that I can clone them and check their case. Do I have to configure such names of the repository here after I create the repositories inside the templates directory? I was expecting a method like create_local_repo which would allow me to create the repositories in templates organisation at runtime. Further, I could clone them and check their case.

I am also not sure whether I have to directly clone the repositories in the template organisation to tmp_path (disk) and check their case. Or first repos setup all repositories corresponding to each student and then clone each of them to check their case.

@slarse
Copy link
Collaborator

slarse commented Jul 5, 2021

This should be executed with templates in the template organization

How does one create more repositories in the template organisation? I think I would need to create templates like TaSK-1, etc. so that I can clone them and check their case.

You just drop a directory into the src/repobee_testhelpers/resources/templates and it's picked up as a template. I did however just come to a realization: file paths on most *NIX system are case sensitive, meaning that the fake platform implementation used in the integration tests (which is just local on the file system) is, wait for it: case sensitive (on most *NIX platforms at least). That's a bit of a problem.

Dynamically, something like this should work (but it doesn't due to silly bug, will work soon):

    def test_setup_with_custom_template_repo(self, platform_dir, platform_url, tmp_path_factory):
        # arrange
        api = funcs.get_api(platform_url, org_name=const.TEMPLATE_ORG_NAME)

        template_repo = api.create_repo("epIcRepo", description="dontcare", private=False)
        local_template_dir = tmp_path_factory.mktemp("template")
        (local_template_dir / "file.txt").write_text("this is a brand new file!")

        branch = "master"
        local_template_repo = funcs.initialize_repo(local_template_dir, default_branch=branch)
        local_template_repo.git.push(api.insert_auth(template_repo.url), branch)
        
        # act
        funcs.run_repobee(f"repos setup -a {template_repo.name} --base-url {platform_url}")

        # assert ...

Now, as I mentioned, this doesn't work because there's a silly bug in the LocalAPI in that it loads its state from a particular file, and that file doesn't change when the organization changes. Oops. Will fix ASAP.

I am also not sure whether I have to directly clone the repositories in the template organisation to tmp_path (disk) and check their case. Or first repos setup all repositories corresponding to each student and then clone each of them to check their case.

We only want to verify that RepoBee operates as expected, and there's no way in RepoBee to clone template repositories individually. These integration tests are fully end-to-end using the CLI, we don't fiddle around with internals in them.

For now, I'd say you can skip the case where there's a template repo on the platform with weird case. Start with just testing that e.g. repos setup with all of the (existing) teplate repos in upper caps works as expected. That is to say, you don't change which templates are available, but you specify them to the --assignments option in upper caps.

Then do that same thing, but with --allow-local-templates.

On a side note, you sure picked a deceptively difficult first issue :D

@algomaster99
Copy link
Contributor Author

there's no way in RepoBee to clone template repositories individually.

Right. That's not the job of repobee anyway.

That is to say, you don't change which templates are available, but you specify them to the --assignments option in upper caps.
Then do that same thing, but with --allow-local-templates.

Sounds good. I will give this a try later today.

On a side note, you sure picked a deceptively difficult first issue :D

I know, right? The title of the issue screams "good-first-issue" but that's surely not the case. 😅

@algomaster99
Copy link
Contributor Author

I will give this a go from scratch and submit a PR soon. I also installed PyCharm today and it helped me understand the flow of the program better. Before today, I was using VSCode 🤦

@algomaster99 algomaster99 deleted the lowercase-repo-name-url branch July 18, 2021 11:21
@slarse
Copy link
Collaborator

slarse commented Jul 18, 2021

Okay! Let me know if you need any assistance. As is evident, this issue turned out to be pretty complicated :)

I've used PyCharm once or twice. It's very nice.

@algomaster99
Copy link
Contributor Author

I've used PyCharm once or twice. It's very nice.

Which IDE/editor do you use for python projects?

@slarse
Copy link
Collaborator

slarse commented Jul 18, 2021

I use Neovim, or Vim if that's the only thing available. I use that for pretty much everything, I really like having one editor that I know very well and can use for anything. I only use an IDE for Java as it's such a ... heavy language.

I'm also not really that into IDEs in general as I find all of the "stuff" to be a little distracting. But that's entirely a matter of taste, I don't think IDEs are bad in any way, and PyCharm is fantastic. Really, all of JetBrains' tools are very 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.

Lowercase all case insensitive values in PlatformAPI
2 participants