Merge pull request #373 from Xronophobe/csanad-review--chapter-25

hjwp · web-flow · commit 1460b64029fc · 2025-04-24T09:57:19.000+01:00
Csanád's review of chapter 25
diff --git a/chapter_25_CI.asciidoc b/chapter_25_CI.asciidoc
@@ -122,6 +122,9 @@ They just don't need any _more_ help dominating the market.
 // DAVID: 1e / 2e abbreviations might confuse people.
 
 So I've decided to use GitLab in this book.
+// CSANAD: I just found framagit.org by Framasoft. Maybe we could mention them? Although
+// it might be important to ask them first, in case they need to handle the
+// expected additional traffic.
 They are absolutely a commercial service,
 but they retain an open-source version, and you can self-host it in theory.
 
@@ -150,7 +153,7 @@ so the first thing to do is get our code up there.
 Use the **New Project** -> **Create Blank Project** option, as in <<gitlab-new-blank-project>>.
 
 //RITA: The screenshot shows that the project name and slug are "Superlistz", yet the next figure  that shows the CI project files on GitLab refers to "lists". Is this correct?
-.Creating a New Repo on Gitlab
+.Creating a New Repo on GitLab
 [[gitlab-new-blank-project]]
 image::images/gitlab_new_blank_project.png["New Blank Project"]
 
@@ -383,7 +386,7 @@ so I know what to expect:  it's failing because Firefox isn't installed
 in the image we're using.
 
 
-Let's modify the script, and an `apt install`.
+Let's modify the script, and add an `apt install`.
 Again we'll do it as late as possible.
 
 [role="sourcecode"]
@@ -717,6 +720,10 @@ status: 0
 
 It takes quite a long time to run,
 and there's lots of debug out, but... it looks OK!
+// CSANAD: I think it ran quite fast, less than maybe 30 seconds. I was confused
+// whether it even worked correctly. I think we should maybe emphasize
+// when it would "look OK": exit status being 0 and 200 OK, I assume?
+
 
 Let's set that environment variable in our CI script:
 
@@ -777,6 +784,9 @@ AssertionError: '2: Use peacock feathers to make a fly' not found in ['1: Buy
 peacock feathers']
  ---------------------------------------------------------------------
 ----
+// CSANAD: we are running the FT-s locally now. I think it would be
+// clearer if we said that explicitly and also if we said a few words on why we
+// are running the FT without docker again.
 
 
 Now you might not see this error,
@@ -843,7 +853,8 @@ SCREEN_DUMP_LOCATION = Path(__file__).absolute().parent / "screendumps"
 
 We first create a directory for our screenshots if necessary.
 Then we iterate through all the open browser tabs and pages,
-and use a Selenium methods, `get_screenshot_as_file()`
+// CSANAD: where? I can't find the iteration through the tabs and pages.
+and use Selenium methods, `get_screenshot_as_file()`
 and the attribute `browser.page_source`,
 for our image and HTML dumps, respectively:
 
@@ -863,6 +874,11 @@ for our image and HTML dumps, respectively:
         path.write_text(self.browser.page_source)
 ----
 ====
+// CSANAD: I suggest placing a newline in front the print:
+//        print("\nscreenshotting to", path)
+// or
+//        print("\n")
+//        print("screenshotting to", path)
 
 
 And finally here's a way of generating a unique filename identifier,
@@ -900,7 +916,18 @@ dumping page HTML to ...goat-book/src/functional_tests/screendumps/MyListsTest.
 test_logged_in_users_lists_are_saved_as_my_lists-2025-02-18T11.29.00.html
 ----
 
-DAVID: Could get them to open those files and have a look? It's kind of satisfying!
+// DAVID: Could get them to open those files and have a look? It's kind of satisfying!
+
+// CSANAD: I just opened them, and they are blank, all-black images. Also, the
+// HTML dump is just:
+// `<html><head></head><body></body></html>`
+// So maybe we could inform the Reader in case they do open these, that this is
+// normal: due to the artificial self.fail() failure, this output is expected.
+
+// CSANAD: however, running all the FT -s tells me something is probably wrong:
+// `$ ./src/manage.py test functional_tests`
+// creates a screenshot and an HTML dump for all FT -s.
+
 
 === Saving Build Outputs (or Debug Files) as Artifacts
 
@@ -937,7 +964,7 @@ test:
 
 
 In any case that should work.
-If you commit the code and then push it back to Gitlab,
+If you commit the code and then push it back to GitLab,
 we should be able to see a new build job.
 
 [role="dofirst-ch25l010-1"]
@@ -953,6 +980,8 @@ $ *git push*
 // so we can check it's working? (Partly it's because you're not so directive about where to put the fail so it
 // feels more ephemeral.)
 
+// CSANAD: the official book-example also removed the `self.fail()` by this point.
+
 In its output, we'll see the screenshots and html dumps being saved:
 
 
@@ -984,6 +1013,21 @@ And if you navigate through, you'll see something like <<gitlab_ui_show_screensh
 [[gitlab_ui_show_screenshot]]
 image::images/gitlab_ui_show_screenshot.png["GitLab UI showing a normal-looking screenshot of the site"]
 
+// CSANAD: working from the book-example, this is not true for me:
+// https://gitlab.com/csberes/superlistz-version-harry/-/jobs/9765894394
+// ```
+// WARNING: src/functional_tests/screendumps/: no matching files. Ensure that the artifact path is relative to the working directory (/builds/csberes/superlistz-version-harry)
+// ERROR: No files to upload
+// Cleaning up project directory and file based variables 00:00
+// Job succeeded
+// ```
+// The path looks OK, and locally, the test does create a `screenshots` directory.
+// The directory does not get created in the Gitlab build job - I guess,
+// because there are no errors. So, even though in my VM the official book-example
+// version does generate a screenshot for every FT, it isn't true for the Gitlab.
+// However, there is another odd thing: I tried re-adding the `self.fail`
+// which resulted in three screenshots on Gitlab:
+// https://gitlab.com/csberes/superlistz-version-harry/-/jobs/9766298077
 
 
 === If in Doubt, Try Bumping the Timeout!
@@ -1093,7 +1137,7 @@ Wrote configuration to spec/support/jasmine-browser.mjs.
 ----
 
 Well we now have about a million files in _node_modules/_
-(which is JavaScript's verions of a virtualenv essentially),
+(which is JavaScript's version of a virtualenv essentially),
 and we also have a new config file in _spec/support/jasmine-browser.mjs_.
 
 That's not the ideal place, because we've said our tests live in a folder called _tests_,
