Add common check: solution identical to exemplar #157

jiegillet · 2021-09-16T08:50:25Z

Covers some features discussed in #132.

This PR:

Introduces a git submodule of the elixir repo
Reads the exemplar solutions to concepts exercises in elixir and compares it to the code, if they are identical, a celebratory comment is added

It does not

Sync the submodule via GH Actions or otherwise
Check that the exemplar solution passes all the tests

This is quite a lot of code, and I have some questions, so this is probably not ready to merge as is. I will leave my questions as comments so it's easier to discuss.

lib/elixir_analyzer.ex

lib/elixir_analyzer/exercise_test.ex

lib/elixir_analyzer/exercise_test/common_checks/exemplar_comparaison.ex

jiegillet · 2021-09-16T09:03:04Z

test/support/exercise_test_case.ex

+         %{"files" => %{"exemplar" => [path]}} <- Jason.decode!(config_file),
+         exemplar_code <-
+           [@concept_exercice_path, slug, path] |> Path.join() |> File.read!() do
+      exemplar_code


Is this readable? I really squeezed the power of with here...

It is not 😁 I think the biggest problem are the multi-step clauses that don't fit on a single line. The mixture of File.read and File.read! is also confusing to me. If this is for testing purposes only, can't everything just raise an error?

Excellent point.

Actually, I spoke too fast, this mustn't crash because it has to return nil for practice exercises.
I'll still rewrite it.

I'm struggling with this. Every ! has a precise reason for being there or not. I settled for adding comments for the moment.

test/support/exercise_test_case.ex

jiegillet · 2021-09-16T09:06:50Z

The tests seem to fail, I guess it's because the submodule isn't loaded... Hopefully it will work locally, you need to run git submodule init then git submodule update --remote --recursive.

jiegillet · 2021-09-22T01:13:56Z

I modified reading the exemplar file according to the discussion with Jeremy, then added tests. There are so many files modified now. Would you prefer I break it down in smaller PRs?

I also realized that I think showing the students errors when the exemplar files are not found or parsed wrong is probably a bad idea, it's better to have it in the logs. We have access to these right?

angelikatyborska · 2021-09-22T09:51:05Z

I'm sorry, I still don't have the time and energy to do a full review, but I'll try to answer some of the questions now.

I also realized that I think showing the students errors when the exemplar files are not found or parsed wrong is probably a bad idea

I agree.

it's better to have it in the logs. We have access to these right?

In theory yes. In practice we only have access to an ad hoc page for maintainers where the logs are visible for a few minutes and then they get pushed out by metadata from other submissions. Only a limited number of most recent submissions are shown, and that limit is platform-wide.

So in practice you need to run a problematic submission and take a look at that page within a minute, otherwise you won't find the log again. I really hope that's something that will be resolved soon. Better debugging tools for maintainers would allow a lot of growth and improvements in all the tracks (cc @iHiD).

I modified reading the exemplar file according to the discussion with Jeremy

I think this also means we need to get rid of the git submodule. Is that possible? Jeremy said he doesn't want analyzer deployments if nothing functional changes in the analyzer. And every commit to main does trigger a deployment, including updating the reference to the submodule, right?

Would you prefer I break it down in smaller PRs?

No need, if they're all part of this one big feature, it's fine.

iHiD · 2021-09-22T11:52:38Z

So in practice you need to run a problematic submission and take a look at that page within a minute, otherwise you won't find the log again. I really hope that's something that will be resolved soon. Better debugging tools for maintainers would allow a lot of growth and improvements in all the tracks (cc @iHiD).

Agreed. There's a very long TODO list atm. Need to find some money, hire some people, and move faster :)

jiegillet · 2021-09-23T10:21:20Z

I'll modify to use logs then.

I think this also means we need to get rid of the git submodule. Is that possible? Jeremy said he doesn't want analyzer deployments if nothing functional changes in the analyzer. And every commit to main does trigger a deployment, including updating the reference to the submodule, right?

I'm still using the submodule for testing all the analyzer checks, since short of copy-pasting, that's the only way we can compare the the exemplar solution. In that sense, we could use the CI to update the submodule on push on elixir-analyzer main, this way it would only update whenever the analyzer would be deploying anyway.

config/test.exs

angelikatyborska · 2021-09-24T14:09:59Z

we could use the CI to update the submodule on push on elixir-analyzer main, this way it would only update whenever the analyzer would be deploying anyway.

Can this be achieved before making the commit on main? Because if we do it after the commit is pushed to main, there will be two deployments :(

angelikatyborska · 2021-09-24T13:33:27Z

lib/elixir_analyzer.ex

+        [path | _] -> Path.join(params.path, path)
+        _ -> nil


Hmm. This line of code assumes that exemplars are always one file and it will ignore the other files if there are more. That seems like asking for trouble. I'm pretty sure that we shouldn't be doing the exemplar check if there are unexpectedly more than one exemplar files, but I am not sure what to do instead. Crash? Log an error and behave as if there was no exemplar at all?

A few line above there was already something similar

relative_code_path = meta_config["files"]["solution"] |> hd()

This ignores anything but the first file and crash if it's empty.

I thought about this a bit more.
For the exemplar code, I guess it's mostly fine because AFAIK, there is only one file for every exercise fo far, and unexpected situations (no files, multiple files) are very unlikely.
For student code, that is more of an issue. Not for website submissions, since I don't think they can create new files, but for CLI submissions, there could be many (Tim loves 3+ files solutions). So the analysis would miss anything outside of the main file.

Am I correct in saying that the directory/file structure in Elixir doesn't matter as long as it's in lib, and that only module names are important?
If that is the case, a simple solution is to concatenate all the files that are in lib (as opposed to meta_config["files"]["solution"] which only points to the stub files) and pretend they were all one file. Actually we can do the same for the exemplar.

test/elixir_analyzer/test_suite/bird_count_test.exs

test/elixir_analyzer_test.exs

test/support/exercise_test_case.ex

angelikatyborska · 2021-09-24T14:06:41Z

test/support/exercise_test_case.ex

+         %{"files" => %{"exemplar" => [path]}} <- Jason.decode!(config_file),
+         exemplar_code <-
+           [@concept_exercice_path, slug, path] |> Path.join() |> File.read!() do
+      exemplar_code


It is not 😁 I think the biggest problem are the multi-step clauses that don't fit on a single line. The mixture of File.read and File.read! is also confusing to me. If this is for testing purposes only, can't everything just raise an error?

lib/elixir_analyzer.ex

test_data/lasagna/missing_exemplar/.meta/design.md

jiegillet · 2021-09-24T15:00:17Z

Can this be achieved before making the commit on main? Because if we do it after the commit is pushed to main, there will be two deployments :(

Really? It would make sense to me that the CI would be run on the push, but before the deployment. For example you may want to have an action that prevents the deployment in some cases. This is just guesswork though, I'll look into it.

iHiD · 2021-09-24T15:53:34Z

Deployment is handled through the GHA on push, so what angelika says is correct. However, presuming the submodule is being ignored in the dockerfile (haven't checked, but it should be) the second run should be a noop. But it would need enough time between the two for that to happen.

Another option would be to update submodules automatically as a PR CI, so it happens on the pull-request action instead where it adds a new commit. It's a messy option though.

jiegillet · 2021-09-25T08:02:02Z

I see.
Would adding a step in the deployment action be an option? I'm assuming you want to keep those identical from repo to repo though.

Another option would be to have this is in the test CI. Maybe we don't need to commit and push the submodule sync, just checking it out would be enough. If we want to run tests locally, we update the submodule locally and commit that along PRs... A bit opaque maybe.

Yet another option is to do without submodules and checking the exemplar from the repo URL, we do that for checking comments exist.

angelikatyborska · 2021-09-25T09:14:12Z

That's a hard dilemma. I'm not sure what's the best course of action here.

This sounds reasonable:

Another option would be to have this is in the test CI. Maybe we don't need to commit and push the submodule sync, just checking it out would be enough. If we want to run tests locally, we update the submodule locally and commit that along PRs... A bit opaque maybe.

But it might cause a situation where the analyzer returns both a celebratory comment and a negative comment. This would happen if we update the analyzer to do some new check but forgot to verify the new check against the most up to date exemplar. Sounds unlikely though, so maybe this is the best option?

This sounds the cleanest and safest:

Yet another option is to do without submodules and checking the exemplar from the repo URL, we do that for checking comments exist.

Except that it will significantly slow down the tests a lot if each test needs to pull the exemplar, or slow down the tests slightly if we git clone the repo once when starting the tests.

jiegillet · 2021-09-25T09:32:42Z

But it might cause a situation where the analyzer returns both a celebratory comment and a negative comment. This would happen if we update the analyzer to do some new check but forgot to verify the new check against the most up to date exemplar. Sounds unlikely though, so maybe this is the best option?

This could happen locally, but then the CI would catch it. That's what it's for. I forget to run mix credo almost all of the time but the CI never forgets :)

Except that it will significantly slow down the tests a lot if each test needs to pull the exemplar, or slow down the tests slightly if we git clone the repo once when starting the tests.

Not a huge deal IMO, the tests on elixir take like 5 minutes. We can tag them with :external so they are not a bother locally.

I know this doesn't help much but I don't really have a preference among all these options...

iHiD · 2021-09-25T13:41:02Z

I'm unsubscribing from this. Re-tag me if you want my thoughts on anything 🙂

angelikatyborska · 2021-09-27T11:51:21Z

This could happen locally, but then the CI would catch it.

Then let's go for that option. To sum up, this is what I think that means:

There is a git submodule for the track repo here.
The submodule is used in unit tests for the new "identical to exemplar" check.
The submodule is not used in production checks because the exemplar is passed as part of the solution.
The submodule doesn't get updated automatically in any way to prevent unnecessary deployments.
On CI, the submodule gets updated before running unit tests to ensure we test against the newest exercise versions, but then the changes are discarded.
If the CI run failed for the above reason, the dev making the PR is expected to also update the submodule reference in their PR.

jiegillet · 2021-09-27T12:30:22Z

That's exactly what I think that means too.
OK then, I'll get started on the CI checks! I think the rest of the code is more or less OK.

Messing with CI with my amount of experience will probably mean some amount of trial and error, so I apologize in advance if I end up spamming commits.

angelikatyborska · 2021-09-27T12:48:23Z

Spamming commits is unfortunately The Only Way ™️ to work with GitHub workflows. I even have a dedicated repo for this for myself :D (https://github.com/angelikatyborska/gha-test)

jiegillet · 2021-09-27T14:17:38Z

Looks like it's working. It only took 14 commits on my fork before I found the solution, which was literally adding with: submodules: true. 🙈

angelikatyborska · 2021-09-28T15:14:37Z

which was literally adding with: submodules: true. 🙈

Is this also going to update the submodule to the latest commit?

jiegillet · 2021-09-28T22:12:49Z

Te

…

Sent from my iPhone

On Sep 29, 2021, at 0:14, Angelika Tyborska ***@***.***> wrote: which was literally adding with: submodules: true. 🙈 Is this also going to update the submodule to the latest commit? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

jiegillet · 2021-09-29T00:18:37Z

Sorry for previous message :)

Is this also going to update the submodule to the latest commit?

Yes, I believe it does. I checked the log of the action and it runs

git submodule sync
git -c http.https://jackfan.us.kg.extraheader="AUTHORIZATION: basic ***" submodule update --init --force

angelikatyborska · 2021-10-02T13:24:16Z

I tried to resolve the conficts and merge but I don't think that's straight-forward, could you do it?

I also noticed a small typo ExemplarComparaison -> ExemplarComparison

jiegillet · 2021-10-02T13:30:44Z

On it!

…#153)

angelikatyborska · 2021-10-03T08:43:47Z

Let's see what happens when I press the merge button 😁

jiegillet requested a review from angelikatyborska as a code owner September 16, 2021 08:50

jiegillet commented Sep 16, 2021

View reviewed changes

jiegillet force-pushed the jie-match-exemplar branch from 159898b to 794f901 Compare September 22, 2021 01:15

jiegillet commented Sep 24, 2021

View reviewed changes

config/test.exs Show resolved Hide resolved

angelikatyborska reviewed Sep 24, 2021

View reviewed changes

test_data/lasagna/missing_exemplar/.meta/design.md Outdated Show resolved Hide resolved

jiegillet mentioned this pull request Sep 25, 2021

Elixir Analyzer Comment: same solution as exemplar exercism/website-copy#2081

Merged

jiegillet added 2 commits October 2, 2021 22:50

Add submodule

48cc35d

Add common check: identical to examplar

db604bb

jiegillet added 14 commits October 2, 2021 22:52

Modify test suite

87ddcc4

Detect assignment on last line of function/macro definition (exercism…

899fa93

…#153)

Add common check: identical to examplar

80fb9ba

include new analysis

638a990

read exemplar from student files instead of submodule

8722e60

Add test for reading student files

a19be94

Remove comments about exemplar files and add log warnings

64e048e

Remove bin files

4218731

Remove pending tags

6d02501

Remove design files

998bfae

Add comments

a37ac3f

Refactor find_exemplar_code

d4f3b67

Add submodule to test CI

f7f38fa

Comparaison -> Comparison

55beb7c

jiegillet force-pushed the jie-match-exemplar branch from 7cdc6d0 to 55beb7c Compare October 2, 2021 13:56

angelikatyborska approved these changes Oct 3, 2021

View reviewed changes

angelikatyborska merged commit ca5fd50 into exercism:main Oct 3, 2021

jiegillet mentioned this pull request Oct 5, 2021

Add high school sweetheart analyzer #186

Merged

jiegillet deleted the jie-match-exemplar branch October 29, 2021 06:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add common check: solution identical to exemplar #157

Add common check: solution identical to exemplar #157

jiegillet commented Sep 16, 2021

jiegillet Sep 16, 2021

angelikatyborska Sep 24, 2021

jiegillet Sep 24, 2021

jiegillet Sep 24, 2021

jiegillet Sep 24, 2021

jiegillet commented Sep 16, 2021 •

edited

Loading

jiegillet commented Sep 22, 2021

angelikatyborska commented Sep 22, 2021

iHiD commented Sep 22, 2021

jiegillet commented Sep 23, 2021

angelikatyborska commented Sep 24, 2021

angelikatyborska Sep 24, 2021

jiegillet Sep 24, 2021

jiegillet Sep 30, 2021

angelikatyborska Sep 24, 2021

jiegillet commented Sep 24, 2021

iHiD commented Sep 24, 2021

jiegillet commented Sep 25, 2021

angelikatyborska commented Sep 25, 2021

jiegillet commented Sep 25, 2021 •

edited

Loading

iHiD commented Sep 25, 2021

angelikatyborska commented Sep 27, 2021

jiegillet commented Sep 27, 2021

angelikatyborska commented Sep 27, 2021

jiegillet commented Sep 27, 2021

angelikatyborska commented Sep 28, 2021

jiegillet commented Sep 28, 2021 via email

jiegillet commented Sep 29, 2021

angelikatyborska commented Oct 2, 2021

jiegillet commented Oct 2, 2021

angelikatyborska commented Oct 3, 2021

Add common check: solution identical to exemplar #157

Add common check: solution identical to exemplar #157

Conversation

jiegillet commented Sep 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiegillet commented Sep 16, 2021 • edited Loading

jiegillet commented Sep 22, 2021

angelikatyborska commented Sep 22, 2021

iHiD commented Sep 22, 2021

jiegillet commented Sep 23, 2021

angelikatyborska commented Sep 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiegillet commented Sep 24, 2021

iHiD commented Sep 24, 2021

jiegillet commented Sep 25, 2021

angelikatyborska commented Sep 25, 2021

jiegillet commented Sep 25, 2021 • edited Loading

iHiD commented Sep 25, 2021

angelikatyborska commented Sep 27, 2021

jiegillet commented Sep 27, 2021

angelikatyborska commented Sep 27, 2021

jiegillet commented Sep 27, 2021

angelikatyborska commented Sep 28, 2021

jiegillet commented Sep 28, 2021 via email

jiegillet commented Sep 29, 2021

angelikatyborska commented Oct 2, 2021

jiegillet commented Oct 2, 2021

angelikatyborska commented Oct 3, 2021

jiegillet commented Sep 16, 2021 •

edited

Loading

jiegillet commented Sep 25, 2021 •

edited

Loading