Project

General

Profile

Feature #97

Feature #66: Write tests

Make tests system parametrizable through configure script

Added by koszko about 2 months ago. Updated about 2 months ago.

Status:
In Progress
Priority:
Normal
Assignee:
Start date:
11/27/2021
Due date:
% Done:

90%

Estimated time:

Description

We should be able to pass at least the following to the configure script:

  • name of pytest executable
  • name of python3 executable
  • name of browser executable (currently hardcoded to /usr/lib/icecat/icecat, for Selenium to work it has to be the actual binary, not a shell script wrapper!)
  • path to an empty profile to that browser (i.e. a profile with all extensions disabled; needed when the operating system has some globally-installed extensions the browser picks up automatically and which could interfere with the tests)
  • port to run the proxy on during tests (current default is 1337)

History

#1

Updated by jahoti about 2 months ago

  • Status changed from New to In Progress
  • Assignee set to jahoti
  • % Done changed from 0 to 90

An attempt at this is now on the jahoti branch (not the build-sys branch, which would be too hard to bring up to date).

A few points of note:

  • the variables used are |Variable | | |---------------|---------------------------------------| |BROWSER_BIN |Path to the browser binary used | |PYTEST |Path to the PyTest binary | |PYTHON |Path to the Python 3 interpreter | |TEST_PORT |Port for the server to use | |TEST_PROFILE |Path to the "empty profile" for testing|
  • The BROWSER_BIN variable sets the browser to use both for testing and for autodetection of the browser type and installation directory. I don't know if this is desirable; the reasoning is that the same makefile is unlikely to be used for testing and installation.
  • In spite of the above, the default values for these two purposes are different; if BROWSER_BIN is not specified, testing will use the hard-coded path to IceCat while autodetection will use the system's default browser.
  • I also added in the make rules for test and test-environment warnings against using a build directory different to the source directory. Hopefully these won't be necessary anyway.
  • Originally, there was also going to be a make rule for building .crx packages included. While I can still add these if you want, it seemed an unnecessary complication as long as they aren't needed for testing (they would require a Chromium-based browser be provided).
#2

Updated by koszko about 2 months ago

Looks great!

non-broken Jahoti's table, for reference:

Variable
BROWSER_BIN Path to the browser binary used
PYTEST Path to the PyTest binary
PYTHON Path to the Python 3 interpreter
TEST_PORT Port for the server to use
TEST_PROFILE Path to the "empty profile" for testing
  • The BROWSER_BIN variable sets the browser to use both for testing and for autodetection of the browser type and installation directory. I don't know if this is desirable; the reasoning is that the same makefile is unlikely to be used for testing and installation.

I'd say it is likely to. That's what would happen in most cases of building a package for a distro (provided we're going to enable tests in this scenario; I assumed we are). How about splitting BROWSER_BIN into BROWSER_BIN and BROWSER and making the first default to

  • the second in case the second is specified and to
  • the current deafault path if neither variable is specified
  • I also added in the make rules for test and test-environment warnings against using a build directory different to the source directory. Hopefully these won't be necessary anyway.

Thanks for bringing the issue to my attention. It seems we can fix it if we modify Python code to allow overriding of the certs dir, right? I can do this later on, when merging your changes (after we resolve the BROWSER_BIN dilemma). Or you can do this if you want.

  • Originally, there was also going to be a make rule for building .crx packages included. While I can still add these if you want, it seemed an unnecessary complication as long as they aren't needed for testing (they would require a Chromium-based browser be provided).

It is certainly not needed right now. Let's postpone it until we make automated tests with Chromium

EDIT
As I am thinking about this now, it seems non-optimal that configure requires the browser we build for to be installed. The auto-detection feature is not bad by itself, I just mean that if the build itself only requires the browser type to be specified and doesn't depend on a browser being currently installed, it'd be good to have configure work in a similar fashion.

Also, doesn't accepting so many different browser types in configure suggest Haketilo would build differently for each of them? I suggest we use just mozilla and chromium targets. The typical configure invocation would then look like:

./configure --host=mozilla BROWSER_BIN=/usr/lib/icecat/icecat'

As a side note, would it make sense to replace options like BROWSER_BIN with ones like --browser-bin? I hope I haven't accidently advocated against GNU standards recommendations by saying this...

#3

Updated by jahoti about 2 months ago

non-broken Jahoti's table, for reference:

Thanks for that! It pays to check what you write afterwards apparently :).

the reasoning is that the same makefile is unlikely to be used for testing and installation.

I'd say it is likely to. That's what would happen in most cases of building a package for a distro (provided we're going to enable tests in this scenario; I assumed we are). How about splitting BROWSER_BIN into BROWSER_BIN and BROWSER and [snip]

Good point. I've just pushed an updated version with this split, using the names BROWSER and TEST_BROWSER (for consistency with other variables).

I also added in the make rules for test and test-environment warnings against using a build directory different to the source directory. Hopefully these won't be necessary anyway.

Thanks for bringing the issue to my attention. It seems we can fix it if we modify Python code to allow overriding of the certs dir, right? I can do this later on, when merging your changes (after we resolve the BROWSER_BIN dilemma). Or you can do this if you want.

In the end it wasn't too much work to fix this- allowing the certs dir to be overridden was the main component- so I ended up doing it in a second commit. The only potential issue is that a test directory is created in the build directory and not removed by any of the make cleaning targets.

As I am thinking about this now, it seems non-optimal that configure requires the browser we build for to be installed. The auto-detection feature is not bad by itself, I just mean that if the build itself only requires the browser type to be specified and doesn't depend on a browser being currently installed, it'd be good to have configure work in a similar fashion.

I'm not entirely sure I understand; however, I'm going to keep working on the configure script today and will definitely modify autodetection behavior. Hopefully it's the way you were thinking; if not, definitely bring this up again!

Also, doesn't accepting so many different browser types in configure suggest Haketilo would build differently for each of them? I suggest we use just mozilla and chromium targets. The typical configure invocation would then look like:

./configure --host=mozilla BROWSER_BIN=/usr/lib/icecat/icecat'

True! There's no real need for the other options anyway with the BROWSER variable; removing the others seems sensible.

As a side note, would it make sense to replace options like BROWSER_BIN with ones like --browser-bin?

Yes- IMHO, they are a much better fit.

I hope I haven't accidently advocated against GNU standards recommendations by saying this...

Unfortunately that was why they were implemented as variables; while I will double-check the standards, the rule seems to be that variables are expected. Supporting both is probably allowed however, and certainly feasible.

#4

Updated by koszko about 2 months ago

I also added in the make rules for test and test-environment warnings against using a build directory different to the source directory. Hopefully these won't be necessary anyway.

Thanks for bringing the issue to my attention. It seems we can fix it if we modify Python code to allow overriding of the certs dir, right? I can do this later on, when merging your changes (after we resolve the BROWSER_BIN dilemma). Or you can do this if you want.

In the end it wasn't too much work to fix this- allowing the certs dir to be overridden was the main component- so I ended up doing it in a second commit. The only potential issue is that a test directory is created in the build directory and not removed by any of the make cleaning targets.

The reason we can't remove it is that if scrdir === builddir, it would also remove our Python code, right? Would a rule like like rmdir test/ 2>/dev/null || true do the job?

As I am thinking about this now, it seems non-optimal that configure requires the browser we build for to be installed. The auto-detection feature is not bad by itself, I just mean that if the build itself only requires the browser type to be specified and doesn't depend on a browser being currently installed, it'd be good to have configure work in a similar fashion.

I'm not entirely sure I understand;

You might have already changed the behavior in your newest commits (I am yet to look into them). Back when I wrote about it, it was like this:

Let's assume a user with IceCat in PATH runs:

./configure --host=icecat
make unpacked

It works and produces a .zip archive with mozilla build of Haketilo without actually invoking IceCat.
Now, let's assume a user without IceCat in PATH runs:

./configure --host=icecat
make unpacked

Now, the configure step fails with realpath being passed an empty argument. Since the actual browser in not a dependency for the build (only for tests), we could expect this to still work.

however, I'm going to keep working on the configure script today

I'll then wait for it before merging to my branch.

Btw, we could also consider adding a flag to enable passing the --verbose option pytest (or maybe just add it always? It shouldn't cause trouble anyway...).

EDIT
Or even -vv instead of --verbose (produces some more output in case of failed tests)

Also available in: Atom PDF