Skip to content
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

Dev-Anleitung #1209

Merged
merged 58 commits into from
Sep 25, 2023
Merged

Dev-Anleitung #1209

merged 58 commits into from
Sep 25, 2023

Conversation

datengraben
Copy link
Contributor

Noch unvollständig

@datengraben datengraben marked this pull request as ready for review April 10, 2023 20:56
* Uses php7.4
* Configures plugin for dev env
* And update Readme.md with development paragraph
@datengraben datengraben added documentation ready PRs that are ready to be merged & only need quick testing / review technical Non-functional changes (refactorings or increase test coverage) labels Apr 12, 2023
@datengraben
Copy link
Contributor Author

@hansmorb @chriwen Bin fertig. Könnt ihr das verstehen oder benutzen? Ich würde es gerne sobald wie möglich mergen. Ist ja quasi eine Anleitung zum Mitmachen.

@hansmorb
Copy link
Contributor

Finde das mit wp-env grundsätzlich schon cool. Gibt es irgendwie ne Möglichkeit direkt phpunit in der richtigen Version mit in das enviroment zu übernehmen?

Und zwei Anmerkungen:

  1. Mein wp-env hat MySQL auf nem anderen Port gestartet als bei dir, ist das immer nen anderer Port? Dadurch ist das Kommando für das installieren der Tests möglicherweise etwas verwirrend.

  2. wp-env run cli wp theme activate kasimir-theme ging bei mir nicht:

image

Und vielleicht könnte man nochmal explizit schreiben, dass man docker für wp-env benötigt. Steht zwar auch in der Doku da drin aber schaden tut es nicht.

@hansmorb
Copy link
Contributor

Und für die Zukunft fände ich es auch mega, wenn man auf der Hauptseite einfach alle Shortcodes hätte und so jeweils ein Artikel, Standort und Timeframe da wäre.

@datengraben
Copy link
Contributor Author

Finde das mit wp-env grundsätzlich schon cool. Gibt es irgendwie ne Möglichkeit direkt phpunit in der richtigen Version mit in das enviroment zu übernehmen?

@hansmorb Was genau meinst du? Phpunit führst du ja lokal aus, also nicht im environment und du musst es vorher noch manuell runterladen.

  1. Mein wp-env hat MySQL auf nem anderen Port gestartet als bei dir, ist das immer nen anderer Port? Dadurch ist das Kommando für das installieren der Tests möglicherweise etwas verwirrend.

Ich habe die Readme angepasst. Ich habe bisher keine Möglichkeit gefunden den DB-Port bei wp-env start zu konfigurieren.

  1. wp-env run cli wp theme activate kasimir-theme ging bei mir nicht:

Das sollte nicht passieren, vielleicht ist vorher was schief gelaufen? Versuche wp-env zurückzusetzen und von einem sauberen Stand aus zu starten.

Und vielleicht könnte man nochmal explizit schreiben, dass man docker für wp-env benötigt. Steht zwar auch in der Doku da drin aber schaden tut es nicht.

Habe ich gemacht.

@datengraben
Copy link
Contributor Author

Und @hansmorb was hälst du davon folgende Plugins noch mit in die wp-env.json zu nehmen?

    "plugins": [ 
      ".",
        "https://downloads.wordpress.org/plugin/wp-crontrol.zip",
      	"https://downloads.wordpress.org/plugin/wordpress-importer.zip",
	"https://downloads.wordpress.org/plugin/query-monitor.zip",
	"https://downloads.wordpress.org/plugin/debug-bar.zip"
    ]

Finde die echt hilreich und sind mir auch jetzt erst über den Weg gelaufen.

@datengraben
Copy link
Contributor Author

Und für die Zukunft fände ich es auch mega, wenn man auf der Hauptseite einfach alle Shortcodes hätte und so jeweils ein Artikel, Standort und Timeframe da wäre.

Wie meinst du das? Nachdem man das Plugin zum ersten Mal installiert hat?

@hansmorb
Copy link
Contributor

Und @hansmorb was hälst du davon folgende Plugins noch mit in die wp-env.json zu nehmen?

    "plugins": [ 
      ".",
        "https://downloads.wordpress.org/plugin/wp-crontrol.zip",
      	"https://downloads.wordpress.org/plugin/wordpress-importer.zip",
	"https://downloads.wordpress.org/plugin/query-monitor.zip",
	"https://downloads.wordpress.org/plugin/debug-bar.zip"
    ]

Finde die echt hilreich und sind mir auch jetzt erst über den Weg gelaufen.

Query monitor UND Debug bar finde ich redundant, die machen ja quasi beide das selbe. Wäre nur für query Monitor weil das auch Debug bar Plugins unterstützt

@datengraben
Copy link
Contributor Author

Query monitor UND Debug bar finde ich redundant, die machen ja quasi beide das selbe. Wäre nur für query Monitor weil das auch Debug bar Plugins unterstützt

@hansmorb Ok ist jetzt drin.

Passt das jetzt? Hat es bei dir geklappt? Den Fehler von dir kann ich bei mir nicht wirklich nachstellen? Hast du mal ein wp-env destroy/kill gemacht?

@hansmorb
Copy link
Contributor

@datengraben Ja, leider immer noch. Auch nachdem ich das Kommando ausgeführt habe.

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #1209 (d574d95) into master (1e7cdef) will increase coverage by 0.18%.
The diff coverage is 89.65%.

@@             Coverage Diff              @@
##             master    #1209      +/-   ##
============================================
+ Coverage     35.61%   35.79%   +0.17%     
- Complexity     2111     2112       +1     
============================================
  Files            83       83              
  Lines          8567     8577      +10     
============================================
+ Hits           3051     3070      +19     
+ Misses         5516     5507       -9     
Files Coverage Δ
src/Plugin.php 28.53% <100.00%> (+4.29%) ⬆️
src/Map/MapShortcode.php 0.00% <0.00%> (ø)
src/Model/Location.php 68.00% <33.33%> (-1.39%) ⬇️

# Conflicts:
#	Readme.md
#	composer.json
#	composer.lock
@hansmorb
Copy link
Contributor

hansmorb commented Sep 7, 2023

Ich hab gerade versucht die Cypress Tests in den Tests ordner zu verschieben und die PHPUnit tests in einen Unterordner namens php bei tests zu verschieben, das hat leider nicht geklappt und ich weiss leider nicht genau wo ich was umstellen soll. Mein Vorbild hier ist ElasticPress https://github.com/10up/ElasticPress/blob/3470e520bbb217e59fea56c2b5647ce43fb42b5f/phpunit.xml.dist

@hansmorb
Copy link
Contributor

hansmorb commented Sep 11, 2023

@datengraben Ich glaube wir müssen in Zukunft alle PRs von Branches vom wielebenwir Repo erstellen, sonst laufen die Tests nicht in der CI durch.

EDIT: Oh, auf einmal geht's doch!

@hansmorb
Copy link
Contributor

Mir immer noch ein Rätsel, warum der Unit Test der die Kartenseite im Backend geladen hat vorher funktioniert hat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hansmorb was meinst du mit "without creating a new instance"? Es läuft doch eine, wenn der Cypress Test ausgeführt wird.

@datengraben
Copy link
Contributor Author

@datengraben Ich glaube wir müssen in Zukunft alle PRs von Branches vom wielebenwir Repo erstellen, sonst laufen die Tests nicht in der CI durch.

EDIT: Oh, auf einmal geht's doch!

@hansmorb ja werde ich aber trotzdem machen. Dann ist alles an einer Stelle.

Copy link
Contributor Author

@datengraben datengraben left a comment

Choose a reason for hiding this comment

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

@hansmorb ich habe nochmal drüberschauen wollen, für offene Punkte. Die Cypress Tests werden jetzt erwähnt, aber das Bauen der Zip fehlt wieder. Ich würde vorschlagen den Branch #1223 reinzumergen, wenn das ok für dich ist. Also dann wäre der Branch für mich fertig zum mergen! :)

```
command in the plugin directory. Make sure that all of your strings use the `__` function with the domain `commonsbooking`. Then you can use `poedit` to open the `commonsbooking-de_DE.po` and update the strings from the `pot` file.

### Build plugin zip
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hansmorb Also diese Stelle stimmt jetzt nicht mehr, das bin/build-zip.sh ist irgendwie rausgeflogen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dann lass uns doch gerne zuerst die #1223 mergen und dann diesen Branch.

@hansmorb hansmorb merged commit 12b6052 into wielebenwir:master Sep 25, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical Non-functional changes (refactorings or increase test coverage)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants