GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Testing by MarkKoz · Pull Request #4 · algorithm-visualizer/tracers.py

Conversation

@MarkKoz

@MarkKoz MarkKoz commented Jul 24, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I did the setup.py before writing tests and then realised I was getting ahead of myself. I had planned to look into using tox to test for 3.5, 3.6, and 3.7 but it may be overkill.

TODO:

  • Add coverage.py dependency
  • Write all tests (100% coverage?)
    • Tracers
      • Array1D
      • Array2D
      • Graph
      • Log
      • Tracer
    • Commander
    • Layouts
    • Randomize
    • JSON output
    • API request
  • Add helper function to drastically reduce repeated code in tests
    • Need to find a way to handle discrepancy in arguments between Commander instances and their keys in command JSON objects.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

64json reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
MarkKoz added 7 commits June 11, 2019 16:37
@MarkKoz
Add packaging provisions
71bf94c
@MarkKoz
Fix circular import
200f91e
@MarkKoz
Make SerializableSequence a generic type
3ebe55d
@MarkKoz
Add mypy and visualization.json to gitignore
7dad08d
@MarkKoz
Fix super()-related TypeError in Array2D.create()
c799cab
@MarkKoz
Fix using same length variable for both dimensions of Array2D
83590a7
@MarkKoz
Add randomizer tests
31e9b53
* Add comments to the Graph randomizer code
@MarkKoz MarkKoz mentioned this pull request Jul 24, 2019
Why can not run tarcers.py locally #3
Open
MarkKoz added 12 commits July 25, 2019 00:14
@MarkKoz
Fix Commander object count tracking
60f8b5d
Modify the class attribute instead of the instance attribute.
@MarkKoz
Add Commander tests
2c3316f
@MarkKoz
Minor refactor to Commander tests
b505a7d
* Use a dictionary for command assertion instead of one assertion per
  kv pair
* Explicitly check for the name "Commander"
@MarkKoz
Add Layout tests
c2728e5
@MarkKoz
Test for undefined arguments for Commander.command()
dcf9599
@MarkKoz
Add Tracer tests
7a43d3d
@MarkKoz
Refactor Commander tests
1c37e4b
* Reset object count and commands list to previous values
* Use dictionaries for all assertions rather than checking single keys
* Create a Commander object in setUp() for use with most tests
@MarkKoz
Add LogTracer tests
818fea1
@MarkKoz
Add Array2DTracer tests
4708103
@MarkKoz
Add Array1DTracer tests
845f70b
@MarkKoz
Return self from GraphTracer.directed()
6ea7a2d
@MarkKoz
Add GraphTracer tests
ae25027
@MarkKoz

MarkKoz commented Jul 28, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Regarding the helper function, there are two approaches I can think of, roughly:

def assertCommandEqual(
    self,
    method: str,
    *args: Union[Serializable, Undefined],
    key: Optional[str] = None
):
    cmd = Commander.commands[-1]
    expected_cmd = {
        "key": key,
        "method": method,
        "args": list(args),
    }

    self.assertEqual(expected_cmd, cmd)

or more comprehensively

def test_command(
    self,
    command: Callable,
    *args: Union[Serializable, Undefined],
    key: Optional[str] = UNDEFINED
):
    if key is UNDEFINED:
        key = command.__self__.key

    # This would need to be done recursively
    args = [arg.key if isinstance(arg, Commander) else arg for arg in args]

    cmd = Commander.commands[-1]
    expected_cmd = {
        "key": key,
        "method": command.__name__,
        "args": args,
    }

    self.assertEqual(expected_cmd, cmd)

And really it could be taken a step further by looping over each tracer and calling the latter helper function for each function in a tracer. Suitable arguments for testing each function could be generated automatically based on type annotations lol. Stupid idea, I know, to rely on function definitions for testing like that.

But I am concerned that the second helper function (and especially that idea at the end) would be a bad approach because it'd effectively remove the "explicitness" of tests. But still, can't help but feel there's so much repetition in the tests even with the first helper function just because the tracers are such thin wrappers.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@64json

64json commented Jul 28, 2019

Copy link
Copy Markdown
Member

I like your idea of randomizing the arguments to test it. 😂 It actually sounds better than the current (manual) way of testing tracers.js.

I was also concerned about the same issue that tests are kinda meaningless at this moment. Should we solely (and more thoroughly) test commander.py and randomize.py? Like testing if it correctly serializes different types of random values, if it prints commands out to visualization.json correctly, etc.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MarkKoz

MarkKoz commented Jul 28, 2019

Copy link
Copy Markdown
Member Author

I don't know what the best way to do this is. I am not really familiar with best practices and methodologies for testing. I do agree there should be some testing on the visualization.json file. As for more thorough testing, I don't know what more can be done for commander.py and randomize.py. It seems to me testing serialisation of various data types doesn't make sense since what we'd really be doing is testing Python's standard library's json module instead of our own code and that wouldn't be necessary unless we want to be paranoid and not trust Python to ship with a working and tested json module.

The problem with my suggestion for parameters based on function definitions is that the tests would rely on the function definitions to work. If a function definition were to be changed to something it's not meant to be, the test wouldn't catch that mistake. I feel like right now the only point of having all the tracer tests is to test for function definitions.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@64json

64json commented Jul 29, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Hmm good points. I think for now we can stick with the current way of testing. Maybe once we have plenty of algorithms translated from js to java/cpp/py, we can run tests on them (that are not using randomized input) to see if they result in the same visualization.jsons?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MarkKoz

MarkKoz commented Jul 30, 2019

Copy link
Copy Markdown
Member Author

Yes, I think some practical usage tests would be good to have. I think we could get away with random inputs if we just seed the RNG.

64json reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MarkKoz added 5 commits August 1, 2019 22:39
@MarkKoz
Add helper function for tests
060fca0
It reduces code redundancy by performing the common operations of
fetching the last command and constructing the command dictionary
from the given arguments.
@MarkKoz
Add missing Graph edge tests
0a69697
@MarkKoz
Configure coverage.py
190e549
@MarkKoz
Refactor execute() to make code more testable
75e641b
* Add type alias equivalent to os.PathLike to support Python 3.5
@MarkKoz
Add a test for create_json_file
9a9168d
@MarkKoz

MarkKoz commented Aug 27, 2019

Copy link
Copy Markdown
Member Author

Could use ideas for how to write a test for get_url. I can test for the return type easily but not sure what else.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@64json

64json commented Aug 27, 2019

Copy link
Copy Markdown
Member

I think testing if the URL returned from get_url responds with 200 would be good.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MarkKoz added 4 commits August 27, 2019 10:45
@MarkKoz
Replace requests with urllib
d7fdacb
@MarkKoz
Add get_url tests
91723d7
@MarkKoz
Mock urlopen for get_url test
cb30d80
@MarkKoz
Exclude atexit from coverage
34724a0
@MarkKoz
MarkKoz marked this pull request as ready for review August 27, 2019 17:59
@MarkKoz

MarkKoz commented Aug 27, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

I think I came up with something decent by mocking some things. I didn't want to test that it returns 200 because that's effectively testing the API, which is out of scope.

This is ready to be reviewed now. I think we can work on tox and automated testing when we're working on setting up CI/CD and packaging.

64json reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

64json
64json approved these changes Sep 3, 2019
View reviewed changes
@64json

64json commented Sep 3, 2019

Copy link
Copy Markdown
Member

It seems great to me. I am no expert in Python so I might miss something. I would appreciate if anyone else reading this thread can review once more.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@64json 64json 64json approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@MarkKoz @64json
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.