- 
                Notifications
    
You must be signed in to change notification settings  - Fork 21
 
Draft: replace nosetests with pytest #936
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
base: main
Are you sure you want to change the base?
Conversation
| return dict() | ||
| 
               | 
          ||
| def news(): | ||
| redirect(URL('default', 'milestones.html')) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was redirecting to a non-existent page
| Test whether the viewer loading functions work | ||
| """ | ||
| @classmethod | ||
| def setUpClass(self): | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spelling of setup/teardown stuff changes across the board
| To carry out tests on a remote machine, you can specify a [url][server] and [url][port] | ||
| in a config file, which will not give the FunctionalTest class an is_local attribute | ||
| and hence will skip tests marked @attr('is_local'). E.g. for testing beta.onezoom.org, you can do | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to choose a solution for running the functional tests on beta.onezoom.org, though it doesn't have to look like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We should record this in a README somewhere.
| selenium_logger.setLevel(logging.WARNING) | ||
| #chrome_options = webdriver.ChromeOptions() | ||
| #chrome_options.add_experimental_option("mobileEmulation", { "deviceName": "iPhone 7" }) | ||
| self.caps = webdriver.ChromeOptions().to_capabilities() | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also updated to selenium 4 whilst we're here, so lots of 'find_element_by...' stuff has changed to the new interface
| print(">> removing temporary appconfig") | ||
| os.remove(self.appconfig_loc) | ||
| 
               | 
          ||
| @tools.nottest | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed some functions so that things we actually want collected as tests start in 'test_' and those that we don't want collected as tests do not
| assert has_linkouts(browser, include_site_internal=True) == False | ||
| assert self.zoom_disabled() | ||
| print(" ", end="", flush=True) | ||
| 
               | 
          
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what this unused function was about. Looks like just an unfinished idea for a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, looks like you are right, or at least that there's a stray print there which we should remove.
| import subprocess | ||
| import urllib.request | ||
| from time import sleep | ||
| #use testconfig from nose (get it using `pip3 install nose-testconfig`) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's where nosetest-testconfig was imported
| pw = getpass("Enter the sql database password: ") | ||
| else: | ||
| pw = match.group(2) | ||
| db['connection'] = pymysql.connect(user=match.group(1), passwd=pw, host=match.group(3), db=match.group(4), port=3306, charset='utf8mb4') | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove before merging, just there for debugging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you remove this?
| 
               | 
          ||
| <ul class = "uk-list columns blockquote-ul"> | ||
| {{category_order={'Scientists': 0, 'Lecturers and professors': 1, 'User quotes': 2, 'Professionals': 3, 'Students': 4, 'Educators': 5} }} | ||
| {{for category in sorted(quotes.keys(), key=lambda x: category_order.index(x) if x in category_order else len(category_order)):}} | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how this is working at all in prod, can anyone enlighten me? @hyanwong? Is there some bygone version of python where dicts have an 'index' method that I don't know about? Is it some web2py shenanigans?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, and it didn't happen that long ago: 42092b6
I'm guessing that index(x) is referring to the list method, i.e. ["moo", "oink", "baa"].index("oink"), whilst category_order was still a list.
@hyanwong should we just revert 42092b6? AFICS the endorsement categories had an explicit order beforehand anyway.
| #only get the ones with an id | ||
| id = elem.get_attribute("id") | ||
| if id: | ||
| yield self.MD_nolinks, id | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just doing something simple for now, but could in principle do some fancier fixture stuff here for better reporting
| 
           @lentinj are thinking that we should just merge this and circle back to fixing any broken tests later. Does that work for you @jaredkhan ?  | 
    
          
 Hey both. I feel we are unlikely to find the motivation to circle back if we don’t resolve things here in this PR, but that’s just an opinion. Either way, should probably at least plug it into the CI if we want to get some value out of these tests going forward.  | 
    
| 
           The main thing stopping us throwing any server-side tests (not just the functional ones) into a CI action is a minimal DB to test against. It's something that we've talked about in the past but I don't think it ever happened (at least, as far as I know). Including a small tree of just Mammals (e.g.) in the OZtree repo would also be useful for getting started with a local instance.  | 
    
- Pass custom appconfig as an environment variable rather than an --arg (which didn't work) - Remove -Q from web2py during functional tests for easier debugging (pytest will still suppress this unless the test fails) - Remove 'href' from =A() calls if links are turned off and we are transforming them to spans - Change the tests to document the current behaviour: invalid, banned, and already-sponsored status overrides the maintenance page
| 
           Nice, thanks. Should I just merge this? Is it ready?  | 
    
          
 @hyanwong its still gonna take quite a bit of work to get this ready since most of the tests still fail. Just taking baby steps on it when I have the time.  | 
    
Work in progress. Will probably need some input from @lentinj and @hyanwong because it looks like these tests haven't been run for a good while and there's lots to fix.
You can still use
grunt exec:test_server_functionalorpython -m pytest tests/functionalto run the testsAbout half of them fail at the moment