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

Add Additional Smoke Tests for Newer Python Features from 3.9, 3.10, 3.11, and 3.12 #50

Open
3 of 9 tasks
BethanyG opened this issue Mar 5, 2024 · 9 comments
Open
3 of 9 tasks

Comments

@BethanyG
Copy link
Member

BethanyG commented Mar 5, 2024

We are largely "safe", now that we've stopped using astor, but we should for completeness add additional smoke tests for features such as:

@brocla
Copy link
Contributor

brocla commented Mar 5, 2024

I'd like to contribute some tests. Perhaps something like this,

    def test_walrus_assignment(self):
        before = """\
                (x := 1)
                (y := 0, 1)
                (z := (0, 1))
                """
        expect = """\
                (placeholder_0 := 1)
                ((placeholder_1 := 0), 1)
                (placeholder_2 := (0, 1))
                """
        self.assertCodeEqual(before, expect)

Would that be helpful?

I can't create a PR yet because the python-representer branches on github are still using astor, not ast.unparse.

@BethanyG
Copy link
Member Author

BethanyG commented Mar 6, 2024

@brocla -- I'd welcome some tests! Two caveats:

  1. We need to wait for Erik to merge PR Representer Updates and Golden Tests #45 and re-run representations on the site. This could take a while, since we need to do that when it won't overload things/cause slowdowns. While we could branch from my branch -- I just don't want to do that. 😬
  2. The tests are a bit fiddly, since we're doing "golden tests". We're essentially recording results, and then comparing new runs to the results that were pre-recorded.

An example test for structural pattern matching can be found here in my fork that the current open PR is based off of.

So the steps would be (updated post-PR 45 merge):

  1. Write some code that uses the feature/function/thing (prefer that this code be similar/analogous to what a student solution might look like)
  2. Create a 'solutions' file, like this example_print_removal.py file.
  3. Run that code through the representer (in docker) to get the 4 output files.
  4. Make sure that those output files are what is expected for the code you wrote.
  5. If everything looks good manually, create a directory for the test under the test/ directory. All of these tests would follow the example naming convention e.g, example-walrus-normalization
  6. Add the 4 output files and the code. Here is an existing one, to follow.
  7. Create the .meta/ directory, and the config.json file inside it. here is one you can copy. Obviously, your name as author, and the name of the file as the 'solution'.

@brocla
Copy link
Contributor

brocla commented Mar 6, 2024 via email

@BethanyG
Copy link
Member Author

BethanyG commented Mar 6, 2024

Hey Kevin,

Looks like Erik merged PR 45 this morning, and the representation re-runs are in process. I think you can go ahead and fork the repo and make a branch for additional tests, since astor is no longer part of main. 😄

Feel free to ping me here, on our Discord server, or on our forum, should you have any questions or issues. My username is the same in all of them.

-Bethany

@brocla
Copy link
Contributor

brocla commented Mar 7, 2024

I posted PR#51, but it was immediately closed.
I was pointed to the community forum. I can post something there if that helps.

Kevin

@kotp
Copy link
Member

kotp commented Mar 7, 2024

I posted #51 , but it was immediately closed. I was pointed to the community forum. I can post something there if that helps.

Kevin

That is the reason that the message says to do that. Linking to the forum post from there and likely mentioning this issue will help keep the information well connected.

I quoted (with an edit that makes the PR link work) to gain the linking mechanism so others can easily click to see what you are referencing.

@BethanyG
Copy link
Member Author

BethanyG commented Mar 7, 2024

@kotp -- we've already pre-agreed (as you can see by the discussion). I am also replying. Posting to the forum is not needed in this curumstance.

@BethanyG
Copy link
Member Author

BethanyG commented Mar 7, 2024

@brocla -- posting to the forum is not needed, since we've already discussed and agreed on the work.

The auto-close and notice are an Exercism-wide policy designed to discourage "drive by" commits by those who aren't clear on how to contribute, or who need to clarify what the work is, and weather or not it is needed/wanted by maintainers.

@BethanyG BethanyG closed this as completed Mar 7, 2024
@BethanyG BethanyG reopened this Mar 7, 2024
@kotp
Copy link
Member

kotp commented Mar 7, 2024

@kotp -- we've already pre-agreed (as you can see by the discussion). I am also replying. Posting to the forum is not needed in this curumstance.

The reply was mostly for the benefit of having a working link to the PR that was mentioned, and in general the reading that is presented being helpful to indicate. If other activity contradicts that, then it is obvious it is not a wanted step. However, then, instead of linking to the forum, linking to the thing that contra-indicates is helpful.

I think we are on the same page here, of course.

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

No branches or pull requests

3 participants