Skip to content

Aut2026review batch2 - #252

Open
mmesiti wants to merge 24 commits into
coderefinery:mainfrom
mmesiti:aut2026review-batch2
Open

mmesiti wants to merge 24 commits into
coderefinery:mainfrom
mmesiti:aut2026review-batch2

Conversation

@mmesiti

@mmesiti mmesiti commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

This is in addition to #246, with another batch of changes to address #245

Pining @Vinye for awareness. Later I will ping maintainers for approval.

Some connection to glossary from the "motivations" episode
removed paragraph
whose topic was
already covered in the table
@mmesiti mmesiti mentioned this pull request Sep 17, 2026
Comment thread content/remotely.md
if you need more control on the running environment.

```{keypoints}
- When fixing bugs or other problems reported in issues, use the issue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This keypoint is a little lonely 💔

Thinking about it, I have trouble seeing the significance of issue auto-closing when compared to the rest of the exercise. Is auto-closing really the most important thing learner should remember from this exercise?

I see two options:

  1. We could add something along the lines of "setting up testing workflows on Github and Gitlab is straightforward with the templates the services provide". Couldn't think right now a better way to summarize the exercise.
  2. Drop the keypoints completely on this page.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do agree:

  • in general the "metadata department" (keypoints and objectives) of the whole lesson is a little underdeveloped, and I think we should try to get to parity with other lessons.
  • in paticular, autoclosing issues is cool because it links nicely with PRs as discussed in week 1, but I do agree this cannot be the only thing here

Opened #254 to this aim

Comment thread content/remotely.md Outdated

- GitHub Actions has a [Marketplace](https://github.com/marketplace?type=actions) which offer wide range of automatic workflows
- On GitLab use [GitLab CI](https://about.gitlab.com/product/continuous-integration/)
- For Windows builds you can also use [Appveyor](https://www.appveyor.com)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Github has runners for all the three major OSes, so I think Appveyor is not the only option. But I don't know about GitLab, I would assume they also have runners. Could be worth mentioning here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do not have experience with appveyor and to my modes experience the appveyor bit of information is too specific and partial, so I think it does not fit here.
Also both github and gitlab have windows runners.

So removed appveyor and expanded on github.com and gitlab.com hosted runners in 4fd8d0d

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

is my approach here too drastic?

Comment thread content/remotely.md
Comment thread content/remotely.md Outdated
- Note that this works also for self-hosted GitLab servers or GitHub Enterprise
(i.e., not gitlab.com or github.com).
Your data does not have to go to a cloud!
- It is also possible to run these workflows on a host

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You got me curious here, how? Do you have a link to point learners to?

@mmesiti mmesiti Sep 19, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you refer to

It is also possible to run these workflows on a host

I'm referring to the GitHub self hosted runners and GitLab runner

I will add these links to the text.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

see 4fd8d0d

Comment thread content/conclusions.md Outdated
Comment thread content/conclusions.md Outdated
Comment thread content/conclusions.md Outdated
This should match (or serve as) an **example in the code documentation** anyway.
- Describe in words how *you* check whether the code still works.
- Translate the words into a script.
- Run the script as often as reasonable

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Inconsistent punctuation throughout all the itemized lists on this page, maybe end all in period?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair point. Will work on that

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

should be done now, consider resolving

Comment thread content/motivation.md Outdated
```

Most scientists nowadays depend on software for research.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe this newline can be removed, so the sentences render as one paragraph.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed newline, but added double space at end so the sentence stays on a single line.
Done in 17a80d2

Comment thread content/motivation.md Outdated
@@ -17,18 +49,14 @@ calibration are compared against the expected response."*

@Vinye Vinye Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I need to try this, I'll see what I can do (I don't like this either, but I haven't found anything better, perhaps epigraph is what I was looking for all the time?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 5cfed98

Comment thread content/motivation.md Outdated
Comment on lines +30 to +59
in order to establish accuracy. Why are we not comparing directly all
digits with the expected result?:
digits with the expected result?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This talk about numeric accuracy should be moved to locally.md as we discussed before.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please see general restructuring of that part in 598f314

Comment thread content/motivation.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This sentence connects poorly to the previous sentences, at least I have hard time parsing what is meant here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please see general restructuring of that part in 598f314

Comment thread content/motivation.md Outdated

```
Very few people are proud of the code they write
the first time they write it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Heh, this reminds me of writing in general, "it's a process" 😁

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We might point this out, that this might sound familiar to anyone making anything original (this applies to art too)

Comment thread content/motivation.md Outdated
@@ -177,13 +209,6 @@ Use the collaborative notes to answer these questions:

## Where to start

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This sections is now an awkward stub when most of the content was removed, maybe remove the whole section?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ehm, I think something went wrong here. I'll have a look.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think that the content of that section are now all covered in the conclusion, so it can be removed.
Fixed in e68959f

@Vinye Vinye left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Made a second pass and added some comments 👀

mmesiti and others added 7 commits September 19, 2026 15:21
Co-Authored-by: Anja Virkkunen <anja.virkkunen@aalto.fi>

Co-authored-by: Michele Mesiti <mmesiti@users.noreply.github.com>
it belongs to conclusions,
where the topic is already covered.
Keeping the italics, though.

As suggested here:
coderefinery#252 (comment)

Co-Authored-by: Anja Virkkunen <anja.virkkunen@aalto.fi>
Comment thread content/motivation.md Outdated
Comment thread content/motivation.md Outdated
Comment on lines +107 to +121
There are many forms of testing.

One can write test programs and run them:
```console
$ python3 run-test.py

running: sample_data/set1.csv --output=tests/set1.txt
CORRECT
```

In the most basic form of a software test,
the observed result is compared with expected result (an "*oracle*")
in order to establish correctness.
Here are some examples of this testing pattern
in different programming languages:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we actually point out that the first one is an end-to-end test and the latter is an example of an unit test? (and links to glossary, just like in the above table)

@mmesiti mmesiti Sep 22, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I had to think a little but you're right:

edited: suggestion as rendered here is misleading, github review suggestions don't work well with backtick blocks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The problem is that I can't fix it here on github. Will fix separately

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

see 5a068cf

Comment thread content/motivation.md Outdated
Comment thread content/conclusions.md Outdated
## The basics: what knowledge do you need?

Learn one test framework well enough for basics:
- Explore and use the good tools that exist out there

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Link learner to quick-reference/#available-tools for a starting point?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oops, it's in the next item, but the link could be changed such that it points to #available-tools instead of the top of the page

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 77f7b50

Comment thread content/conclusions.md Outdated

@Vinye Vinye left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Took a look again, made some new observations 🔍

Also, just to make sure, sphinx has flagged some lines, I assume you have seen those warnings?

Comment thread content/motivation.md Outdated
Comment thread content/remotely.md Outdated
@mmesiti
mmesiti force-pushed the aut2026review-batch2 branch 3 times, most recently from 4b52849 to 801d877 Compare September 22, 2026 17:10
mmesiti and others added 2 commits September 22, 2026 19:12
Co-Authored-by: Anja Virkkunen <anja.virkkunen@aalto.fi>
I think it fits better there,
after we have shown why automated tests are important.
Also before showing automated tests
many of the "typical problems"
might sound abstruse or unfamiliar.
@mmesiti
mmesiti force-pushed the aut2026review-batch2 branch from 801d877 to 6a2db30 Compare September 22, 2026 17:12
mmesiti and others added 3 commits September 22, 2026 19:25
Co-Authored-by: Anja Virkkunen <anja.virkkunen@aalto.fi>
Co-Authored-by: Anja Virkkunen <anja.virkkunen@aalto.fi>
@mmesiti
mmesiti force-pushed the aut2026review-batch2 branch from 0817bc7 to 04abe9a Compare September 22, 2026 17:31
@mmesiti

mmesiti commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor Author

Sorry for all the force-pushing. I think I almost addressed all the comments so far... Still some more work to come.

Thanks @Vinye for the awesome feedback (GitHub should have an award for the most patient reviewer on earth)!

@Vinye Vinye left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I went through the texts with a fine-tooth comb once more (I'd like to use a Finnish idiom here: read it like the devil reads the Bible 👿 ), so expect a lot of typo and stylistic fixes.

Moreover, I suggested fixes for the Sphinx warnings in quick-reference.md (some extraneous spaces on new lines). The Sphinx warning in conclusions.md on the other hand could be fixed by adding myst_heading_anchors = 3 to the conf.py file, see here. The link works even without this, but this would silence the warning.

Comment thread content/conclusions.md Outdated
Comment thread content/conclusions.md Outdated
Comment thread content/conclusions.md Outdated
Comment thread content/conclusions.md Outdated
Comment on lines +38 to +41

Fortunately for us,
someone has already found a solution for most of these
and created {term}`testing framework`s that we can use

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Fortunately for us,
someone has already found a solution for most of these
and created {term}`testing framework`s that we can use
Fortunately for us,
someone has already found a solution for most of these
and created {term}`testing framework`s that we can use.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This looks to me like a repeat of the previous fix, so I won't apply it

Comment thread content/conclusions.md
Comment on lines +42 to +44

Note: not all frameworks solve all problems
(also because sometimes the underlying language does not have the necessary features).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd move this note below the table (now we are giving a note about problems before we present the problems in the table)

@mmesiti mmesiti Sep 23, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd argue against this just because this table is not meant to be discussed in detail, it's more there as a collection of reasons why one should use a testing framework. The message "... but not all frameworks tackle all the problems" is perhaps more important than the details contained in the table.

Perhaps, we should move the whole table to an appendix (or just the "quick-reference" episode, which isn't that quick anymore), put a link here, and go through it if permits? Not sure about that though.

Comment thread content/remotely.md Outdated
Comment thread content/remotely.md Outdated
Comment thread content/remotely.md Outdated
Comment thread content/remotely.md Outdated
Comment thread content/remotely.md Outdated
mmesiti and others added 5 commits September 23, 2026 14:57
Punctuation, Jargon, Capitalization, repeated words and most importantly sphinx warning errors

Co-authored-by: Anja <Vinye@users.noreply.github.com>
Co-authored-by: Michele Mesiti <mmesiti@users.noreply.github.com>
also, made title of forge feature comparison table
more sober.
Co-Authored-by: Anja Virkkunen <anja.virkkunen@aalto.fi>
slimmer columns for readability
@mmesiti

mmesiti commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Moreover, I suggested fixes for the Sphinx warnings in quick-reference.md (some extraneous spaces on new lines). The Sphinx warning in conclusions.md on the other hand could be fixed by adding myst_heading_anchors = 3 to the conf.py file, see here. The link works even without this, but this would silence the warning.

Thank you for pointing out where the problems were. Eventually I added a manual anchor to fix the warning, probably less invasive than changing the conf.py (I think it makes sense to keep the conf.py as uniform as possible across the lessons).

@mmesiti
mmesiti marked this pull request as ready for review September 23, 2026 19:04
Adding a picture here might be unnecessary
since there's so many that one can find online.
false positive is a bug is flagged when not present
false negative is when a bug is there but it's not flagged.

Notice that tests can only be used to prove incorrectness,
not to prove correctness,
so the condition we are testing for is the presence of bugs,
not its absence.
@mmesiti

mmesiti commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Also with dfb8fb7 corrected this issue that I would like to stress:

local testing: false negative -> false positive

false positive is a bug is flagged when not present
false negative is when a bug is there but it's not flagged.

Notice that tests can only be used to prove incorrectness,
not to prove correctness,
so the condition we are testing for is the presence of bugs,
not its absence.

@mmesiti

mmesiti commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

I think that, apart from the test design episode, these are all the changes that I would make. Thanks @Vinye for the review and I'd like @johanhellsvik to have a final look, I don't expecting making other changes to the general structure of the lesson.

(I think that the test design episode might deserve separate PRs, which are partially already there)

This branch has not been deployed

No deployments
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.

2 participants